[libcxx-commits] [PATCH] D100595: [libcxx][test] Attempt to make debug mode tests more bulletproof

Kristina Bessonova via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon May 17 03:14:40 PDT 2021


krisb added inline comments.


================
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");
 
----------------
Quuxplusone wrote:
> 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.
Agreed


================
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");
 
----------------
Quuxplusone wrote:
> 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;
> }
> ```
I'd leave this out of the scope of the patch. This patch is already large enough, so I'm not sure this is a good idea to include more NFCs here, especially those that are not closely related to the initial intent of the patch.
Except for the changes for `TEST_LIBCPP_ASSERT_FAILURE` macro I'd:
- fix correctness issues,
- remove redundant includes (since most became redundant because of this patch),
- simplify whatever within `TEST_LIBCPP_ASSERT_FAILURE`.
All other improvements/simplifications are good for follow-ups. What do you think?


================
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");
----------------
Quuxplusone wrote:
> 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
> ```
I'd leave this out of the scope of this patch. We have similar tests for all the containers, and they tested the specific functionality at least at the moment they were added (I haven't checked how things are going now, though).


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