Improve Container Types #180
Conversation
| if (aSize > Capacity()) | ||
| Reserve(aSize); | ||
|
|
There was a problem hiding this comment.
| if (aSize > Capacity()) | |
| Reserve(aSize); | |
| if (aSize > Capacity()) | |
| { | |
| Reserve(aSize); | |
| } |
- Fixed issues - Added more methods (Erase, Insert, etc.) - Refactored various methods - Improved semantics - Generally cleaned up
|
Id still like to have a look if I may |
|
|
||
| DynArray& operator=(DynArray&& aOther) noexcept | ||
| { | ||
| if (this != std::addressof(aOther)) |
There was a problem hiding this comment.
Why not simply swap contents for moving ops btw? Should be cleaner and shorter code overall. Also no need to check for this etc. in such instance.
There was a problem hiding this comment.
I believe it was @wopss who wanted it this way, as it doesn't introduce any complexity
There was a problem hiding this comment.
I don't remember complaining about this. I also don't get why we don't do std::swap.
| } | ||
|
|
||
| auto distance = std::distance(aFirst, aLast); | ||
| DifferenceType newSize = std::abs(distance); |
There was a problem hiding this comment.
I dont like allowing negative values, shouldnt be the case for proper range. Only case this should ever return negative is for random access range where you swapped first and last, shouldnt affect reverse iterators on the range AFAIK.
There was a problem hiding this comment.
Im actually unsure if we should even care for distance, IMO for generic case like this when iterators/range isnt random access, it should just use back_inserter, this may introduce tons of unwanted overhead. Iteration over some types isnt fast, and iterating through it twice unnecessarily may bottleneck things unless it is random access range as mentioned.
There was a problem hiding this comment.
How do you suggest I avoid the double evaluation?
I assume it involves changing the template concept to std::forward_iterator?
There was a problem hiding this comment.
No, use std::back_inserter instead of directly using begin/end for copying. And preallocate space only when you have std::random_access_iterator via reserving and not resizing. For std::random_access_iterator, std::distance is constant operation.
| } | ||
|
|
||
| template<std::input_iterator InputIt> | ||
| void Assign(InputIt aFirst, InputIt aLast) |
There was a problem hiding this comment.
Im missing some way to assign range directly, we are long past the age of having to use iterators unless absolutely necessary. Can achieve practically everything with ranges and views more cleanly.
There was a problem hiding this comment.
Applies to constructors also.
There was a problem hiding this comment.
Missing some move-assignment.
There was a problem hiding this comment.
What are you suggesting here? Should I create a new method to accept a range, or adjust this one? If it's the former, what structures are implied by ranges and views?
There was a problem hiding this comment.
New method ideally which accepts any range as a type. Ranges have to have begin() and end() defined, other functions like empty() and size() are not guaranteed unless you go to specializations of ranges.
It likely would end up as a wrapper for iterator-based assignments.
|
|
||
| void PushBack(ConstReference aItem) | ||
| { | ||
| EmplaceBack(std::forward<ConstReference>(aItem)); |
There was a problem hiding this comment.
I dont think std::forward is correct in this instance. Likely wont cause an issue but that is more thanks to its definition, shouldnt be ever needed when you know you are passing lvalue reference.
|
|
||
| void PushBack(ValueType&& aItem) | ||
| { | ||
| EmplaceBack(std::forward<ValueType&&>(aItem)); |
There was a problem hiding this comment.
This almost certainly seems wrong, forwarding && will always result in rvalue reference even when it formerly wasnt. Just ValueType should be enough.
There was a problem hiding this comment.
I don't even see the need of std::forward here, a simple std::move should be enough.
| { | ||
| assert(aPos < End() && Includes(aPos)); | ||
|
|
||
| aPos->~ValueType(); |
|
|
||
| [[nodiscard]] constexpr SizeType MaxSize() const noexcept | ||
| { | ||
| #undef max // avoid conflict with minmax |
There was a problem hiding this comment.
This shouldnt be needed provided NOMINMAX macro is correctly defined. And if you are worrying about this, rather wrap min and max keywords in parenthesis eg. std::numeric_limits<SizeType>::(max)()
There was a problem hiding this comment.
I would be against providing NOMINMAX macro since others might not like it when this library is used in their code.
| template<std::input_iterator InputIt> | ||
| StaticArray(InputIt aFirst, InputIt aLast) | ||
| { | ||
| if (std::distance(aFirst, aLast) > MaxSize()) |
There was a problem hiding this comment.
And here you dont check the size and make it absolute and whatnot. Just as a note to the other distance comment I had, negative values are practically never a good sign.
There was a problem hiding this comment.
I just thought about this, but removing this header and class will cause a lot of issues. So, I propose to add it back, deprecate them (the header and the class).
There was a problem hiding this comment.
What kind of issues? The changes already include updates to all the DynArray file paths, and Reflection-inl.hpp was updated accordingly for the dumper
There was a problem hiding this comment.
People project's still use this path, changing it like that will be a breaking change.
There was a problem hiding this comment.
Simplest solution to this - just have file in original location including the file from right location.
Isnt likely as clean, but should be more than sufficient imo not @wopss ?
Also the least amount of additional changes.
There was a problem hiding this comment.
Exactly. And add a pragma message in the header saying that it is deprecated.
No description provided.