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] Transition InlinedFixedVecor.type to CollectionElement #3561

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from

Conversation

rd4com
Copy link
Contributor

@rd4com rd4com commented Sep 28, 2024

This commit is a possible transition for InlinedFixedVector,

changing it's type parameter from AnyTrivialRegType to CollectionElement.

It is for a feature request by @JoeLoser:

#3478


Not sure if it is exactly what you had in mind @JoeLoser,

but here is a possible implementation 👍

@rd4com rd4com requested a review from a team as a code owner September 28, 2024 19:40
@rd4com rd4com force-pushed the InlinedFixedVector_CollectionElement branch from 454037f to 2e75886 Compare September 28, 2024 20:27
Signed-off-by: rd4com <144297616+rd4com@users.noreply.github.com>
@rd4com rd4com force-pushed the InlinedFixedVector_CollectionElement branch from 2e75886 to 0b6ed60 Compare September 28, 2024 21:16
@szbergeron
Copy link
Contributor

Would it be possible to use conditional conformance within __del__ to only even emit the destruction code if the type isn't trivial? I know one of the wants from this type was originally that it be almost entirely trivial to move/copy (and would be if it doesn't overflow the inline vec)

Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

Looks great! Just needs a rebase and a minor organization comment I made wrt to the tests.

Thanks for tackling this!

mutability: Bool, //,
type: CollectionElement,
static_size: Int,
lifetime: Lifetime[mutability].type,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion You'll need to rebase since s/Lifetime/Origin recently

self.current_size = 0
self.capacity = capacity

@always_inline
fn __init__(inout self, existing: Self):
fn __copyinit__(inout self, existing: Self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion If you'd like, we could make this conform to ExplicitlyCopyable too.


var vector = InlinedFixedVector[MyStruct, 16](1000)

# assert del
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion Instead of having one long test function with these conceptual different cases, from a maintenance perspective and ease of readability for others, I'd recommend putting each of these test cases in their own function, like so:

def test_destructor():
  ...

def copy_constructor():
  ...

def test_move_constructor():
  ...

def test_move_constructor_no_dynamic():
  ...

and so on. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants