Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Constexpr-ify inplace_vector #21
Constexpr-ify inplace_vector #21
Changes from 10 commits
48d7243
ba31ac9
f8517de
cc75d2a
8799d0b
cfbc617
446426c
3ba7896
431b0f1
12fe05e
541ad68
6b75318
8dbc733
5ff463a
3ec2600
64fdedc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason you are using
std::launder
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be consistent with previous implementation, I am bit confused myself too. I am not in a good place to make judgement call here, though I suspect it might be needed when T is const.
I don't think we should change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I will mark a TODO here. There's a lot of non-conformity in this implementation.I think we should defer this to another PR, there's too much to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but I need to make some remark on all constructors in
inplace_vector_destruct_base
.IMO, it would be O.K. to address this in a later PR.
They all value-initialise
elems
, i.e. the bytes in bytes_based_Storage become 0 and all elements in type_based_storage are value initialised. I tried to argue above why I don't like this, they should be initialised only when a vector element is really constructed in the place.Besides (superfluously) initialising
elems
, they all do the same thing, they setsize_
to some value. That is, they avoid doing what a copy c'tor, move c'tor etc. normally does and instead manipulatesize_
only. This is redundant. Instead, a single constructor taking asize
would do. This could be called by the all the constructors in the deriving class.size_
could probably be made private.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inplace_vector_destruct_base
needs more work on trivially destructible requirement as well, needs to switch between a trivially destructible base and a non-trivial destructor against T.Unfortunately intention in this repo is to support C++17, so we cannot use requires. This have to be done with template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but to me this is unconditionally noexcept, since no T's are moved at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but to me this is unconditionally noexcept, since no T's are copied at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but needs to be an
uninitialised_copy
, in particular for the byte based storage case.It happens work in the type based storage case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is
uninitialised_copy
needed here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, but must be
uninitalized_move
, see above.