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] Give the CollectionElementNew trait to UnsafeMaybeUninitalized #2971

Conversation

gabrieldemarmiesse
Copy link
Contributor

This is part of the PR #2965

@ConnorGray I think you're the right person for a review :)

…alized`

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@gabrieldemarmiesse gabrieldemarmiesse requested a review from a team as a code owner June 6, 2024 18:28
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@gabrieldemarmiesse
Copy link
Contributor Author

gabrieldemarmiesse commented Jun 11, 2024

@ConnorGray all done. I also think that it should be possible in a future PR to remove the requirements for the type that can be put inside an UnsafeMaybeUninitialized. And also to use "constrained" instead of abort

EDIT: Actually, I'll clean up more stuff in this PR.

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

Summary of the changes:

  1. UnsafeMaybeUninitialized now takes AnyType as element. Added very specific traits to each method to allow this.
  2. UnsafeMaybeUninitialized now has the CollectionElementNew trait, but none of the methods of this trait can actually be called because of ambiguity, and will fail at compile time.
  3. Switch from abort to constrained for a faster fail. It's a nice QoL feature.
  4. Removed inout from initialize_pointee_explicit_copy as we are not mutating the pointer but the value it points to. This is because, without this change, we can't do self.unsafe_ptr().initialize_pointee_explicit_copy(...) because it's mutating an rvalue, which is not allowed by the compiler (rightfully or not, I don't have an opinion on this).
  5. Moved ValueDestructorRecorder in the file stdlib/test/test_utils/types.mojo where it can be used in other test files, it's a very useful struct.

@ConnorGray it's ready for review, for real this time :)

@ConnorGray
Copy link
Collaborator

!sync

" UnsafeMaybeUninitialized. It has very specific semantics."
),
]()
self = Self()
Copy link
Collaborator

@ConnorGray ConnorGray Jun 13, 2024

Choose a reason for hiding this comment

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

I think making this a compile time failure might prevent uses like InlineArray[UnsafeMaybeUninitialized[T]], because that type would instantiate a call to this copy constructor.

The goal would be to avoid dynamically doing a copy operation, but we need the compiler to accept code that "looks like" it could do a copy, because its up to the programmer to make sure no actual copy gets attempted.

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Jun 13, 2024
@@ -343,7 +343,7 @@ struct UnsafePointer[
@always_inline
fn initialize_pointee_explicit_copy[
T2: ExplicitlyCopyable
](inout self: UnsafePointer[T, address_space], value: T2):
](self: UnsafePointer[T, address_space], value: T2):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 , this makes sense.

@ConnorGray
Copy link
Collaborator

Hey Gabriel, thanks for your work on this, I think this all looks good—I like that change to make this work with AnyType.

Re: the constrainted[False] change, I think that will make InlineArray[UnsafeMaybeUninitialized[T]] fail to compile. Considering fixing InlineArray / InlineList was the motivation for this type, I think we might to stick with abort for the time being.

We could investigate switching this to constrained[False] maybe in a different PR. (Or maybe even removing the __init__(inout self, Self) and __moveinit__ constructors entirely?)

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@ConnorGray
Copy link
Collaborator

Awesome, thank you for the changes Gabriel! This looks good, I'll move forward with merging :)

@ConnorGray
Copy link
Collaborator

!sync

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label Jun 13, 2024
modularbot pushed a commit that referenced this pull request Jun 14, 2024
…aybeUninitalized` (#41760)

[External] [stdlib] Give the `CollectionElementNew` trait to
`UnsafeMaybeUninitalized`

This is part of the PR #2965

Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com>
Closes #2971
MODULAR_ORIG_COMMIT_REV_ID: 994417a3cb7a2b160fb44f4455cd8f71de03af8b
@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Jun 14, 2024
@modularbot
Copy link
Collaborator

Landed in 431a620! Thank you for your contribution 🎉

@modularbot modularbot closed this Jun 14, 2024
modularbot pushed a commit that referenced this pull request Sep 13, 2024
…aybeUninitalized` (#41760)

[External] [stdlib] Give the `CollectionElementNew` trait to
`UnsafeMaybeUninitalized`

This is part of the PR #2965

Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com>
Closes #2971
MODULAR_ORIG_COMMIT_REV_ID: 994417a3cb7a2b160fb44f4455cd8f71de03af8b
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants