Page MenuHomePhabricator

[libcxx][test][NFC] Various tests for std::vector
ClosedPublic

Authored by kboyarinov on Oct 25 2021, 4:32 AM.

Details

Summary

Add missing tests for std::vector funcionality to improve code coverage:

  • Access tests were rewritten to test modification of the container using the reference returned by the non-const overload
  • Added tests for reverse iterators: rbegin, rend, etc.
  • Added exception test for vector::reserve
  • Extended test cases for vector copy assignment
  • Fixed insert_iter_value.pass.cpp to use insert overload with const value_type& (not with value_type&& which is tested in iter_rvalue.pass.cpp test)

Diff Detail

Event Timeline

kboyarinov requested review of this revision.Oct 25 2021, 4:32 AM
kboyarinov created this revision.
libcxx/test/std/containers/sequences/vector/access.pass.cpp
32

Here again, I think the default argument is never used (and if it is, please change the caller).

59

The default argument is never used. (Or if it is and I missed it, please just change that caller to pass , 0 explicitly.)
https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/

96–97

Ah, here's the caller that should change to test_get<Vector>(0);.
But also, can you explain why you are testing with both 0 and 36? What would you expect to "go wrong" with the 0 case — why not just test with 36 alone? (And then you wouldn't even need to pass N at all; it could be a constant.)

libcxx/test/std/containers/sequences/vector/reverse_iterators.pass.cpp
29–33

The body of std::distance(i, j) is just going to return j - i; so I think you should just check that expression directly. Or, don't even bother, because the reverse_iterator tests should (I hope) be dealing with that operator.
If you omit it, then these 13 lines can become 3, which is great:

Vector vec;
assert(vec.rbegin() == vec.rend());
assert(vec.crbegin() == vec.crend());

Do we have any existing test for ASSERT_SAME_TYPE(typename Vector::reverse_iterator, std::reverse_iterator<typename Vector::iterator>)? If not, I'd add that here.

libcxx/test/std/containers/sequences/vector/vector.capacity/reserve.pass.cpp
59

Nit throughout: Catch by const reference.

libcxx/test/std/containers/sequences/vector/vector.modifiers/emplace_extra.pass.cpp
61–72

Having variables with the same name as methods is confusing (and verbose). I think this can just be

std::vector<int> v;
v.reserve(8);
size_t oldCapacity = v.capacity();
assert(oldCapacity >= 8);
v.resize(4);  // keep the existing capacity
assert(v.capacity() == oldCapacity);
v.emplace(v.cend(), 42);
assert(v.size() == 5);
assert(v.capacity() == oldCapacity);

Also notice that simplifying the code found a bug: after v.reserve(8) it is not necessarily true that v.capacity() == 8. The capacity might be greater than 8.

libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_lvalue.pass.cpp
54

Same comment here: at least rename size to n.

59–61

This seems gratuitous... Ah, you're aiming for something subtle with value categories: trying to ensure that the thing being inserted is an lvalue. If I've guessed correctly, then please s/value/lvalue/g and also rename this test to insert_iter_lvalue.pass.cpp.

Fix comments from differential review

kboyarinov marked 7 inline comments as done.Nov 17 2021, 12:24 AM
kboyarinov added inline comments.
libcxx/test/std/containers/sequences/vector/access.pass.cpp
96–97

I have double-checked and it seems like testing with 0 is not necessary. Thanks for catching this!

libcxx/test/std/containers/sequences/vector/reverse_iterators.pass.cpp
29–33

Removed std::distance for empty vector test-case.

Do we have any existing test for ASSERT_SAME_TYPE(typename Vector::reverse_iterator, std::reverse_iterator<typename Vector::iterator>)? If not, I'd add that here.

We have such a test in types.pass.cpp for vector.

libcxx/test/std/containers/sequences/vector/vector.modifiers/emplace_extra.pass.cpp
61–72

Applied. Thank you for catching an issue with capacity!

libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_lvalue.pass.cpp
59–61

You are correct, currently we have 2 overloads for std::vector::insert: one takes an argument as const value_type&, an other - as value_type&&. This test is intended to test the first one, but actually tests the second. We have the test insert_iter_rvalue.pass.cpp for the second overload.

If I've guessed correctly, then please s/value/lvalue/g and also rename this test to insert_iter_lvalue.pass.cpp.

I have renamed this test to insert_iter_lvalue.pass.cpp. Could you please clarify what you mean with s/value/lvalue/g?

kboyarinov marked 3 inline comments as done.Nov 17 2021, 12:25 AM
libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_lvalue.pass.cpp
59–61

s/value/lvalue/g — "replace value with lvalue, not stopping at the first occurrence."
Renaming int value to int lvalue will help the reader understand that the value category is important here.

Renaming value to lvalue for insert_iter_lvalue.pass.cpp

kboyarinov marked 2 inline comments as done.Nov 17 2021, 10:21 PM
kboyarinov added inline comments.
libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_lvalue.pass.cpp
59–61

Thank you for the clarification! I have renamed the variable across the file.

kboyarinov marked an inline comment as done.Nov 17 2021, 10:21 PM
ldionne resigned from this revision.Nov 18 2021, 5:35 AM

Deferring approval to Arthur who seem to have taken a deep look. Thanks for improving the tests!

Quuxplusone requested changes to this revision.Nov 18 2021, 8:51 AM

I still have a few more nits.

libcxx/test/std/containers/sequences/vector/access.pass.cpp
116–117

Today I wonder, what do you expect to be the salient difference between int and EqComparable?
If you'd used MoveOnly instead of EqComparable, I would have said "okay, I guess that makes sense to test," but EqComparable... it's still copyable, it's still trivial... basically the only difference is that it doesn't support operator< (which has nothing to do with this test). I don't see why we need it.

libcxx/test/std/containers/sequences/vector/reverse_iterators.pass.cpp
47–55

Instead of this loop, I believe you can just do

assert(cvec.rbegin() == vec.crbegin());
assert(cvec.rend() == vec.crend());
70

Might as well assert(std::distance(cvec.crbegin(), cvec.crend()) == 10); to complete the foursome.

libcxx/test/std/containers/sequences/vector/vector.capacity/reserve.pass.cpp
77

I suggest s/0/42/ here and line 65, just to make sure that the vector didn't magically get filled with zeros specifically.
Alternatively, assuming .data() is C++03, just do

int *previous_data = v.data();
size_t previous_capacity = v.capacity();
[...]
assert(v.size() == 10);
assert(v.capacity() == previous_capacity);
assert(v.data() == previous_data);

(and actually I'd say old_ rather than previous_, for brevity)

libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
37

In this case, do we expect other_allocator::operator= to be called, or not?
If we don't verify our expectation, then I think this new test is useless — it's equivalent to either lines 22-26 or lines 29-33, but we don't know which.
How can we verify that the behavior is as we expect?
(Do we have a test allocator type where "identity" is distinguishable from "equality"? I seem to recall we do, but I haven't looked.)

This revision now requires changes to proceed.Nov 18 2021, 8:51 AM

Fix comments from the last revision of the review

kboyarinov marked 3 inline comments as done.Nov 22 2021, 2:24 AM
kboyarinov added inline comments.
libcxx/test/std/containers/sequences/vector/vector.capacity/reserve.pass.cpp
77

I have changed 0 value to 42 and also added an assertion with v.data().
I did not removed the loop that checks values inside of the vector to ensure not only that reserve did not modified the memory (v.data()) but also did not changed the actual values inside of that memory.

libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
37

We expect allocator::operator= to be called only if allocator_traits<allocator>::propagate_on_container_copy_assignment::value is true.

I have replaced this test-case with the following three:

  • POCCA false (Allocator is A1 from allocators.h)
  • POCCA true and the allocators compares equal (Allocator is A3)
  • POCCA true and the allocators compares unequal (Allocator is A3)

This change also required A1 and A3 to provide allocate and deallocate that provides real memory. Absence of these method seemed really strange for me.

kboyarinov marked 2 inline comments as not done.Nov 22 2021, 2:25 AM

A1 and A3 class templates from allocator.h cannot be used to test propagate_on_container_copy_assignment option during the vector::operator=(const vector&).
I have tried to change allocate and deallocate methods in these allocators to use new-delete to provide the real memory, but tests for std::scoped_allocator_adaptor has failed.
I have introduced the new class POCCAAllocator<T, bool> to test the behavior of vector on copy assignment.
Tests for other containers can also be rewritten to use the new allocator class.

LGTM % comments... tempted to mark this "accepted," but let's go around one more time. ;)

libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
47

With my suggestion below, this becomes

using Alloc = NonPOCCAAllocator<int>;
bool copy_assigned_into = false;
std::vector<int, Alloc> l(3, 2, Alloc(5, &copy_assigned_into));
std::vector<int, Alloc> l2(l, Alloc(3, nullptr));
l2 = l;
assert(!copy_assigned_into);
assert(l2 == l);
assert(l2.get_allocator() == Alloc(3, nullptr));

For the cases below where we expect assert(copy_assigned_into), it might be a good idea to assert(!copy_assigned_into) on the line immediately before the assignment, just to be safe; but I'm not married to that idea.

libcxx/test/support/allocators.h
188

Please remove the default argument. Besides that unused defaults are just typo-bug-magnets, even if you were using this default, it would be the wrong default — surely a POCCAAllocator<int> should be POCCA!
I also notice that you felt the need to /*comment=*/ on each use of this class, which indicates there's room for improvement. Since this is C++11-only, how about creating two alias templates

template<class T>
using POCCAAllocator = MaybePOCCAAllocator<T, true>;
template<class T>
using NonPOCCAAllocator = MaybePOCCAAllocator<T, false>;

and using them consistently throughout?
Finally, static bool copy_assign_called; is a libcxx/test/ antipattern, because it's not constexpr-friendly. Could we easily make that a non-static bool *copy_assigned_into_ and set it up in the constructor like we set up id_, to point to a stack variable controlled by the caller? I'll give an example usage above.

197

Default function arguments being the devil, please spend 2 lines on this. :)

explicit POCCAAllocator() = default;
explicit POCCAAllocator(int id, bool *p) : id_(id), copy_assigned_into_(p) {}
~~~
int id_ = 0;
bool *copy_assigned_into_ = nullptr;

Removed static bool variable, renamed POCCAAllocator, added aliases

kboyarinov marked 5 inline comments as done.Nov 22 2021, 12:05 PM
kboyarinov added inline comments.
libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
47

Applied, thank you for the great suggestion!

libcxx/test/support/allocators.h
197

I have splitted the constructor into default, the constructor that accepts id only and the constructor with id and bool*.

kboyarinov marked 2 inline comments as done.Nov 22 2021, 12:05 PM
Quuxplusone accepted this revision.Nov 22 2021, 12:32 PM
Quuxplusone added inline comments.
libcxx/test/std/containers/sequences/vector/vector.cons/assign_copy.pass.cpp
47

I notice you added a third constructor Alloc(int) so that you could write Alloc(3) instead of Alloc(3, nullptr). I don't think this buys you enough (in terms of characters saved) to offset its cost (in terms of risk-of-typo-bug-where-you-omit-the-parameter-accidentally). So I'd prefer to eliminate that ctor and pass nullptr explicitly in these ~6 places.

libcxx/test/support/allocators.h
206

After this line, arguably you should do copy_assigned_into_ = a.copy_assigned_into_... but I guess I can see it either way, and the existing tests don't care at all, so I'm happy to leave it like this. Just bringing it up for the record. No action required.

This revision is now accepted and ready to land.Nov 22 2021, 12:32 PM
kboyarinov edited the summary of this revision. (Show Details)

Removed the constructor of MaybePOCCAAllocator from int argument

kboyarinov marked an inline comment as done.Nov 22 2021, 12:49 PM

Overall, looks good to me, but I believe some small improvements could be done.

libcxx/test/std/containers/sequences/vector/access.pass.cpp
42

Here we rely that the size of vector is 10. But it goes from outer context. IMO, it's error prone and not very convenient for further maintenance. I would suggest to either pass the size from outer context or just read it from Vector object depending on what's more appropriate for the test purpose.

72

Here is the similar situation. Even if we have 10 in the same context refactoring of this code in not as good as might be. I would suggest to either create a variable for the size (as you do in one of the other tests) or call c.size()

kboyarinov marked 2 inline comments as done.Nov 29 2021, 1:23 AM
rarutyun added inline comments.Dec 1 2021, 4:55 AM
libcxx/test/std/containers/sequences/vector/reverse_iterators.pass.cpp
63

Here the comment with easier refactoring is not addressed yet.

kboyarinov updated this revision to Diff 390982.Dec 1 2021, 5:09 AM

Changed reverse_iterators.pass.cpp to use int n = 10 instead of n in all places

kboyarinov marked an inline comment as done.Dec 1 2021, 5:09 AM
rarutyun accepted this revision.Dec 1 2021, 5:55 AM

LGTM, waiting for CI to complete

This revision was automatically updated to reflect the committed changes.