Skip to content

Conversation

@20162026
Copy link
Collaborator

@20162026 20162026 commented May 10, 2025

implement:

Fixes #64
Fixes #81

@20162026 20162026 requested a review from a team May 10, 2025 22:30
@20162026
Copy link
Collaborator Author

20162026 commented May 10, 2025

There were already some tests for AssignInitializerList but the compiler probably substituted the il assignment with something else (il constructor?), so they passed successfully even without the implementation

T, std::ranges::range_reference_t<std::initializer_list<T>>> &&
std::movable<T>)
{
clear();
Copy link
Member

Choose a reason for hiding this comment

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

surely this is a pessimisation if the current is smaller than than the il -- of course we can't get the size of the initializer list because sometimes we're really dumb...

however we could perform the insert first and then only clear the elements needed -- hmm -- yeah idk this might be better. ugh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont think it matters much as for trivial type clear is just a setsize and for nontrivial it is setsize+destructor call (no memory allocation or other potential overhead).

Copy link
Member

@JeffGarland JeffGarland May 10, 2025

Choose a reason for hiding this comment

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

well currently there's no optimization for trivial types -- which is a whole thing. it actually destroys all the elements.

edit: no it just erases, which doesn't destruct

edit2: So why isn't this simply assign_range( il );

Copy link
Collaborator Author

@20162026 20162026 May 10, 2025

Choose a reason for hiding this comment

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

yeah I will fix that, it definatly should have been assign_range...

also constexpr iterator insert(const_iterator position, InputIterator first, InputIterator last) implementation might not be correct, but I will investigate it tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

edit: no it just erases, which doesn't destruct

What do you mean here

Copy link
Member

@JeffGarland JeffGarland left a comment

Choose a reason for hiding this comment

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

I'm approving even though I have one outstanding comment. This is better than it was and I'm sure there will be more passes on this code for further possible optimizations.

Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

thank you for your contribution

@wusatosi wusatosi merged commit 0b5bfc5 into bemanproject:main May 15, 2025
25 checks passed
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.

operator=(initializer_list) is not implemented try_append_range is not implemented

3 participants