Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stdlib] Use UnsafePointer in vector.mojo #2377

Closed

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented Apr 22, 2024

With this change I get a weird error:

 ./stdlib/scripts/run-tests.sh && lit -sv stdlib/test
Creating build directory for building the stdlib running the tests in.
Packaging up the Standard Library.
Included from /projects/open_source/mojo/stdlib/src/collections/__init__.mojo:1:
/projects/open_source/mojo/stdlib/src/collections/vector.mojo:250:48: error: '_VecIter' parameter #2 has 'fn(UnsafePointer[InlinedFixedVector[type, size], 0], Int, /) -> type' type, but value has type 'fn(selfptr: UnsafePointer[InlinedFixedVector[type, size], 0], i: Int, /) -> type'
    alias _iterator = _VecIter[type, Self, Self._deref_iter_impl]
                                           ~~~~^~~~~~~~~~~~~~~~~
/projects/open_source/mojo/stdlib/src/__init__.mojo:1:1: note: '_VecIter' declared here
# ===----------------------------------------------------------------------=== #
^
mojo: error: failed to parse the provided Mojo source module

That does seem like a compiler bug no? Those looks the same to me:

fn(UnsafePointer[InlinedFixedVector[type, size], 0], Int, /) -> type
fn(selfptr: UnsafePointer[InlinedFixedVector[type, size], 0], i: Int, /) -> type

Maybe I'm missing something?

@Moosems
Copy link

Moosems commented Apr 22, 2024

Not the same, one has a named argument ((sarcasm) yeah, I think it's a compiler bug).

@gabrieldemarmiesse gabrieldemarmiesse changed the title [stdlib] Use UnsafePointer in simd.mojo and vector.mojo [stdlib] Use UnsafePointer in vector.mojo Apr 25, 2024
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@JoeLoser
Copy link
Collaborator

Thanks for pushing on this! I'll import the PR internally and we'll take a look at the compiler bug and see if there's a workaround, etc.

@JoeLoser JoeLoser added the imported-internally Signals that a given pull request has been imported internally. label Apr 27, 2024
@lattner
Copy link
Collaborator

lattner commented May 5, 2024

Thanks for poking me about this Joe. @gabrieldemarmiesse the problem here is a disagreement about AnyRegType, I think it works if you change VecIter to:

@value
struct _VecIter[
    type: AnyRegType,
    vec_type: AnyType,
    deref: fn (UnsafePointer[vec_type], Int) -> type,

AnyRegType is a bad thing :), we need to move off it completely to use AnyType.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@gabrieldemarmiesse
Copy link
Contributor Author

Thanks @lattner it works well :) This PR is ready for review now

@JoeLoser JoeLoser added the merged-internally Indicates that this pull request has been merged internally label May 5, 2024
@@ -243,5 +243,5 @@ struct InlinedFixedVector[
An iterator to the start of the vector.
"""
return Self._iterator(
0, self.current_size, Reference(self).get_legacy_pointer()
0, self.current_size, UnsafePointer(Reference(self))
Copy link
Contributor

@soraros soraros May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

Suggested change
0, self.current_size, UnsafePointer(Reference(self))
0, self.current_size, UnsafePointer.address_of(self)

@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
JoeLoser pushed a commit that referenced this pull request May 6, 2024
[External] [stdlib] Use `UnsafePointer` in `vector.mojo`

As part of the migrating from `Pointer` to `UnsafePointer`,
continue this in `vector.mojo`.  This requires changing one bit
to use `AnyType` rather than `AnyRegType` to play nicely
with `UnsafePointer`.  In the near future, we want to replace
use of `AnyRegType` with `AnyType` throughout the library.

---------

Co-authored-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Closes #2377
MODULAR_ORIG_COMMIT_REV_ID: a9d3d17ea2dc2e092569829d632754c7c81a6a39
@JoeLoser
Copy link
Collaborator

JoeLoser commented May 7, 2024

Landed in #2559!

@JoeLoser JoeLoser closed this May 7, 2024
@JoeLoser JoeLoser added the merged-externally Merged externally in public mojo repo label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally mojo-repo Tag all issues with this label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants