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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:21
 
 #include <list>
 #include <cstdlib>
----------------
krisb wrote:
> curdeius wrote:
> > krisb wrote:
> > > 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?
> > > 
> > I think it's important to keep these tests with `-D_LIBCPP_DEBUG=1/2` as @krisb indicated because they should pass in debug mode. However, as their purpose isn't clear, it would be wise to add a comment.
> Well, from the perspective of D100866 it looks okay to switch these tests from debug to normal mode, so that 'debug mode tests' are assumed to be only negative tests. But if this is acceptable, I'd prefer to do this in a follow-up patch. 
I agree: once D100866 lands, these tests won't really have anything to do with a "debug mode." They simply test a property of the move-constructor of `std::list` (namely iterator non-invalidation), which is mandated by the Standard to work. And then the new buildkite job in D100866 will verify that this property holds both in regular mode //and// in debug mode.

This test should be removed, and we should add a legitimate test for iterator (and pointer) non-invalidation in-or-alongside these files:
- libcxx/test/std/containers/sequences/list/list.cons/move.pass.cpp
- libcxx/test/std/containers/sequences/list/list.special/swap.pass.cpp

I'm fine with saying that we can do that in a separate PR. However, honestly, I think it'd be good to submit that PR //now//, get it merged //now//, and then //after// that we can update this PR to simply delete this file.
If you really really want to defer that for later, then OK but please add a `FIXME` comment in this file explaining the situation and pointing at move.pass.cpp and swap.pass.cpp, so that we physically cannot forget about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595