[libcxx-commits] [PATCH] D100595: [libcxx][test] Attempt to make debug mode tests more bulletproof
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu May 13 14:57:24 PDT 2021
Quuxplusone added a comment.
These tests sure are tedious. :) I may have more comments later, but I figure I should post these for now.
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.modifiers/emplace_db1.pass.cpp:23-36
class A
{
int i_;
double d_;
A(const A&);
A& operator=(const A&);
----------------
Could we replace this whole bit with
```
struct A {
explicit A(int i, double d) {}
};
```
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.modifiers/erase_iter_iter_db1.pass.cpp:27
std::list<int> l2(a1, a1+3);
- std::list<int>::iterator i = l1.erase(l2.cbegin(), next(l1.cbegin()));
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(l1.erase(l2.cbegin(), next(l1.cbegin())),
+ "list::erase(iterator, iterator) called with an iterator not referring to this list");
----------------
`std::next` (and ditto throughout)
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.modifiers/insert_iter_iter_iter_db1.pass.cpp:29-31
const int N = sizeof(a)/sizeof(a[0]);
- std::list<int>::iterator i = v.insert(next(v2.cbegin(), 10),
- cpp17_input_iterator<const int*>(a),
- cpp17_input_iterator<const int*>(a+N));
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(v.insert(next(v2.cbegin(), 10), cpp17_input_iterator<const int*>(a),
+ cpp17_input_iterator<const int*>(a + N)),
----------------
Let's make this simply
```
TEST_LIBCPP_ASSERT_FAILURE(v.insert(v2.cbegin(), a, a+5),
"list::insert(iterator, range) called with an iterator not referring to this list");
```
(and remove `test_iterators.h`)
I see no reason for `std::next`, `N`, or `cpp17_input_iterator` to be involved here.
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.modifiers/insert_iter_size_value_db1.pass.cpp:26-27
std::list<int> c2;
- std::list<int>::iterator i = c1.insert(next(c2.cbegin(), 10), 5, 1);
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(c1.insert(next(c2.cbegin(), 10), 5, 1),
+ "Attempted to increment a non-incrementable list::const_iterator");
----------------
Check me on this, but I'm pretty sure this is testing the wrong thing. It looks like it should be more like
```
c1.insert(c2.cbegin(), 5, 1)
```
with a message about an iterator not referring to this list.
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.modifiers/pop_back_db1.pass.cpp:33
assert(c.empty());
- c.pop_back(); // operation under test
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(c.pop_back(), "list::pop_back() called on an empty list");
----------------
Now that we run the regular tests in debug mode (so that non-failing `pop_back`s are already well tested), we can simplify this test to just
```
int main(int, char**)
{
std::list<int> c;
TEST_LIBCPP_ASSERT_FAILURE(c.pop_back(), "list::pop_back() called on an empty list");
return 0;
}
```
================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.ops/db_splice_pos_list_iter.pass.cpp:29
+ v1.splice(v1.begin(), v2, v1.begin()),
+ "list::splice(iterator, list, iterator) called with the second iterator not referring to list argument");
}
----------------
FWIW, the error message here is still missing an article: "referring to //the// list argument"?
Ditto in "db_splice_pos_list_iter_iter.pass.cpp".
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/db_back.pass.cpp:24-29
typedef int T;
typedef std::vector<T> C;
C c(1);
assert(c.back() == 0);
c.clear();
+ TEST_LIBCPP_ASSERT_FAILURE(c.back(), "back() called on an empty vector");
----------------
This can be just
```
int main(int, char**)
{
std::vector<int> c;
TEST_LIBCPP_ASSERT_FAILURE(c.back(), "back() called on an empty vector");
return 0;
}
```
Please make the substitution throughout this directory, anywhere you see `typedef int T;` being used trivially.
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/db_back_2.pass.cpp:24-30
int main(int, char**) {
typedef int T;
typedef std::vector<T, min_allocator<T> > C;
C c(1);
assert(c.back() == 0);
c.clear();
+ TEST_LIBCPP_ASSERT_FAILURE(c.back(), "back() called on an empty vector");
----------------
This test seems the same as the previous...? Ah, I see, this one uses `vector<int, min_allocator<int>>` instead of `vector<int>`. However, IMHO we can just kill this test; it's not significantly different from the previous one. I recommend
```
git rm db_back_2.pass.cpp
git rm db_cback_2.pass.cpp
git rm db_front_2.pass.cpp
git rm db_cfront_2.pass.cpp
git rm db_index_2.pass.cpp
git rm db_cindex_2.pass.cpp
```
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/db_front.pass.cpp:23-29
int main(int, char**) {
typedef int T;
typedef std::vector<T> C;
C c(1);
assert(c.front() == 0);
c.clear();
+ TEST_LIBCPP_ASSERT_FAILURE(c.front(), "front() called on an empty vector");
----------------
As above:
```
int main(int, char**)
{
std::vector<int> c;
TEST_LIBCPP_ASSERT_FAILURE(c.front(), "front() called on an empty vector");
return 0;
}
```
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/db_index.pass.cpp:29
c.clear();
- assert(c[0] == 0);
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(c[0], "vector[] index out of bounds");
----------------
On these `operator[]` tests, I'd prefer a non-empty vector: more like
```
int main(int, char**)
{
std::vector<int> c(5);
TEST_LIBCPP_ASSERT_FAILURE(c[5], "vector[] index out of bounds");
return 0;
}
```
================
Comment at: libcxx/test/libcxx/containers/unord/unord.multimap/db_swap_1.pass.cpp:37-40
c2.erase(i1);
std::unordered_multimap<int, int>::iterator j = i1;
- c1.erase(i1);
- assert(false);
+ TEST_LIBCPP_ASSERT_FAILURE(
+ c1.erase(i1), "unordered container erase(iterator) called with an iterator not referring to this container");
----------------
There's a couple of things wrong here IMHO:
- `c2.erase(i1)` invalidates iterator `i1`, so even trying to copy its value into `j` seems sketchy.
- `j` is unused.
- `c1.erase(i1)` uses an invalid iterator; I think it should use a valid iterator.
These issues could all be addressed by doing this:
```
int main(int, char**)
{
typedef std::pair<int, int> P;
P a1[] = {P(1, 1), P(3, 3), P(7, 7), P(9, 9), P(10, 10)};
P a2[] = {P(0, 0), P(2, 2), P(4, 4), P(5, 5), P(6, 6), P(8, 8), P(11, 11)};
std::unordered_multimap<int, int> c1(a1, a1+5);
std::unordered_multimap<int, int> c2(a2, a2+7);
std::unordered_multimap<int, int>::iterator i1 = c1.begin();
std::unordered_multimap<int, int>::iterator i2 = c2.begin();
swap(c1, c2);
c1.erase(i2);
TEST_LIBCPP_ASSERT_FAILURE(c1.erase(i1),
"unordered container erase(iterator) called with an iterator not referring to this container");
return 0;
}
```
Ditto on the other unordered containers' `db_swap_1` tests.
================
Comment at: libcxx/test/libcxx/utilities/optional/optional.object/optional.object.observe/dereference_const_rvalue.pass.cpp:24
int main(int, char**) {
std::optional<int> opt;
+ TEST_LIBCPP_ASSERT_FAILURE(*std::move(opt), "optional operator* called on a disengaged value");
----------------
`const std::optional<int> opt = std::nullopt;`
Judging from the filename, the `const` should be there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100595/new/
https://reviews.llvm.org/D100595
More information about the libcxx-commits
mailing list