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

Mon Apr 19 15:39:19 PDT 2021

krisb added a comment.

In D100595#2698871 <https://reviews.llvm.org/D100595#2698871>, @Quuxplusone wrote:

>> Rebase on main + D100029 <https://reviews.llvm.org/D100029> + D100592 <https://reviews.llvm.org/D100592>, remove unused typedefs, update after D100728 <https://reviews.llvm.org/D100728>
>
> My impression was that D100029 <https://reviews.llvm.org/D100029> was obsolete and should be abandoned, with any remaining bits rolled into D100595 <https://reviews.llvm.org/D100595>.
> D100592 <https://reviews.llvm.org/D100592> looks ready to land whenever you feel like it; I hope it's not blocked on D100029 <https://reviews.llvm.org/D100029> (and if it is, I suggest you figure out how to extricate it from that dependency).

That's not quite right. D100029 <https://reviews.llvm.org/D100029> is still actual, as it ensures that the tested containers have at least one element, which seems to be assumed when the tests were written. Otherwise, we would need to adjust the tests to check against empty containers.
D100592 <https://reviews.llvm.org/D100592> isn't blocked by anything, so I'll land it once the pre-commit tests pass.

================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:21

 #include <list>
 #include <cstdlib>
----------------
ldionne wrote:
> curdeius wrote:
> > It seems to me that other headers are unnecessary here.
> Now this makes more sense to me - we just don't want to overwrite the `_LIBCPP_ASSERT` macro.
> 
> However, shouldn't we now rename this test since it's not related to the libc++ debug mode anymore? And while we're at it, maybe we don't need this test anymore if it's already tested in the tests for `std::list`?
It doesn't seem like we have some normal mode tests that check that iterators keep valid after moving. But I think it can still be valuable in debug mode (I mean with `-D_LIBCPP_DEBUG=1` defined) as it also checks that debug mode correctly handles cases like this and doesn't trigger any assertions (which would be triggered, for example, if there were two different containers). Does it make sense?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100595/new/

https://reviews.llvm.org/D100595