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

[Kristina Bessonova via Phabricator via libcxx-commits]

krisb added inline comments.


================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:21
 
 #include <list>
 #include <cstdlib>
----------------
Quuxplusone wrote:
> 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.
Since we have a couple more similar tests that should be checked and converted to regular mode, I'd prefer to do this after D100866 gets landed, so we will be able to run new tests in both regular and debug modes with ease.
Having this, I'd not block this patch by D100866 and focus on issues in debug mode tests first. So, if you okay with this, I'd prefer to add FIXMEs and proceed with other changes from this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595