[libcxx-commits] [libcxx] [libc++][vector] Make constructor vector(count, value, allocator) exception-safe (PR #82033)

[Mateusz Zych via libcxx-commits]

mtezych wrote:

Sure, I've created new pull request #82068 with the change that
inlines the remaining constructors filling vector with the same value.

I hope that placing those constructors physically next to each other
will make code better, since they all have nearly identical implementation,
which needs to be kept in sync.

---------------------------------------------------------------------------------------------------------------------------

Regarding the unit-tests, yes, of course, I agree that all changes fixing bugs
should also introduce tests verifying these bugs were indeed fixed and will never happen again.

I even tried to extend your tests [//libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp),
by adding test cases covering the `vector(count, value, allocator)` constructor.

However, these exception safety tests covering constructors of `std::vector<>` are so full of problems,
that I didn't even know where to start in order to fix them. In no particular order:

- The correctness of all test cases is verified only by calling `check_new_delete_called()`,
which only checks whether operators `new` and `delete` were called equal number of times.
Sure, this can to some extent verify whether vector's data buffer was allocated and freed correctly,
but it definitely does not verify whether the vector's elements have been constructed and destructed correctly.

- Some of test cases, which supposed to verify whether a constructor of `std::vector<>` behaves correctly,
when an exception is thrown while copying the allocator parameter, are in fact, not correct,
because the Allocator class is defines non-throwing copy constructor:

    - [Throw in vector(size_type, const allocator_type&) from allocator](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L128)
    - [Throw in vector(InputIterator, InputIterator, const allocator_type&) from allocator](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L175)
    - [Throw in vector(InputIterator, InputIterator, const allocator_type&) from allocator](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L184)

- All test cases verifying correctness of `std::vector` constructors in situations,
when an exception is thrown during construction of an element, are operating on single element vectors.
This is a problem, because such test cases cannot verify whether destructors of already successfully created elements
will indeed be invoked, when not all elements could have been successfully created.

- The `ThrowingT` class has reference semantics, since it holds a pointer to a counter,
which determines when to throw an exception. I think that changing this class to represent a value type
would make these unit-tests much easier to understand and extend in the future.

- In some test cases `std::vector<>` instances are named `get_alloc`. What's that about?

    - [`std::vector<ThrowingT> get_alloc(1);`](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L114)
    - [`std::vector<ThrowingT> get_alloc(1, v);`](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L123)
    - [`AllocVec get_alloc(0, alloc);`](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L130)

- Two test cases are available only, when they are compiled in C++11 mode,
and yet C++11 features are used freely, without any `#ifdef`s.
Which minimal version of the C++ standard can I assume?

    - [`#if TEST_STD_VER >= 11`](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L220)
    - [`using AllocVec = std::vector<int, Allocator<int> >;`](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp#L106)

In my opinion the above issues need to be resolved first, then the test cases covering this bug fix can be added.

I am perfectly happy to do so, but this will require rewriting almost the whole [exceptions.pass.cpp](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp) source file,
so I don't want to do it without asking you first whether this is indeed the desired direction.

https://github.com/llvm/llvm-project/pull/82033