Left shift in StaticVector::erase(), add tests

[Yggdrasill]

Hello,

While I was working on using StaticVector for vendor item arrays, I noticed that StaticVector::erase() doesn't seem to behave as expected. I asked about this on Discord and @glebm concluded it is a bug, and says that it is intended to work exactly like std::vector's erase.

The following changes have been made to static_vector.hpp:

• std::move is called before std::destroy_at, shifting items left.
• std::destroy_at() now destroys the entries at the back of the array after the move.
• StaticVector::clear() returns early if there are no member items.

I am not an expert in C++ and am not 100% confident in this PR or the changes made, so feedback is greatly appreciated.

Regards,
Yggdrasill

[chasedjones88]

Consider using the term "Shift" instead of "Shuffle" as shuffle indicates, at least to me, an element of random reordering.

[Yggdrasill]

Sorry for the messy commit history. I always forget to run clang-format before pushing, and MSVC produced some test failures that I can't produce locally on Debian. I set up a VM to run these tests locally now.

There is at this point a possible change that I'd like to discuss here. Currently StaticVector::erase() returns early if erasures are requested out of bounds, and leaves the array untouched. What I would like to ask is if it's desired that erase() should clamp these requests to the bounds of the array, and operate normally. When called with either:

container.erase(container.begin() - 1);
container.erase(container.end() - 1, container.end() + 1);

Would it be better to return early as it does presently, or should it fix up these values to within bounds? Or should it perhaps do an assertion for these cases?

[AJenbo]

I think we both made the mistake thinking this would be a quick little change :D

[ephphatha]

Would it be better to return early as it does presently, or should it fix up these values to within bounds? Or should it perhaps do an assertion for these cases?

Adding asserts will be fine, I don't know exactly where in the spec it's defined but usually the expectation is the caller will do the right thing and not try erase invalid values.

For example, you can assert(element >= begin() && element < end()) inside erase(T *element) to match the expected behaviour of std::vector.

container.erase(container.begin() - 1);

would then trigger an assert failure (or crash in release builds).

and for erase(T *first, T* last) assert(first >= begin() && last <= end() && first <= last) is sufficient.

container.erase(container.end() - 1, container.end() + 1);

would fail the assert

container.erase(container.begin(), container.begin()) would be fine and result in a no-op, even if the container is empty.

[Yggdrasill]

Adding asserts will be fine, I don't know exactly where in the spec it's defined but usually the expectation is the caller will do the right thing and not try erase invalid values.

I've added the asserts and also added test cases for them, which are disabled in release builds. In debug builds the tests will also test assertions.

[ephphatha]

Looks good, one more nit and pointing out something you did that I didn't think of but makes sense for the other test case too :)

[ephphatha]

Nice work, sorry for the back and forth for what is a fairly isolated change 😅

looks correct to me but it’s getting late in the evening so I’ll hope a second set of eyes can do the merge (or I’ll check again tomorrow)

[Yggdrasill]

I appreciate the review, don't worry about the back and forth. Thank you.

[Yggdrasill]

So I've researched this more, discussed this some more on Discord, and looked at a libstdc++ implementation of std::vector:

    _M_erase(iterator __position)
    {
      if (__position + 1 != end())
	_GLIBCXX_MOVE3(__position + 1, end(), __position);
      --this->_M_impl._M_finish;
      _Alloc_traits::destroy(this->_M_impl, this->_M_impl._M_finish);
      _GLIBCXX_ASAN_ANNOTATE_SHRINK(1);
      return __position;
    }

This code moves [__position + 1, end()) to __position, then shrinks the size of the vector, destroys the tail element and tells the address sanitiser that the size shrunk by one. I have closely mirrored this in the new commit, and it seems like the correct implementation.

In the new commit, the elements being moved to are overwritten, both with move assignable and copy assignable types. The elements trailing at the end are now garbage, and need to be cleaned up, where std::destroy comes in. The elements that shouldn't be erased have already been moved, and std::move takes care of cleaning up loose ends like that, so it really shouldn't be a problem. Conceptually, it is somewhat similar to this C code, as far as move assignable types are concerned:

int *ptr1 = malloc(sizeof(*ptr1));
int *ptr2 = malloc(sizeof(*ptr2));
free(ptr1);
ptr1 = ptr2;
ptr2 = NULL;

The build is failing right now, but only because of a CMake update in the build environment.