[libcxx-commits] [PATCH] D100592: [libcxx][test] Split off debug mode tests

[Kristina Bessonova via Phabricator via libcxx-commits]

krisb added inline comments.


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/db_back.pass.cpp:32-33
+  c.clear();
+  assert(c.back() == 0);
+  assert(false);
+
----------------
Quuxplusone wrote:
> IIUC, this can/should be simply
> ```
> (void)c.back();  // expect this to exit
> assert(false);
> ```
> But as long as you fix it in D100595, it's no problem to leave it wonky in this commit.
Yes, I'd prefer to leave the changes like this for D100595.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_9.pass.cpp:14
 // UNSUPPORTED: libcxx-no-debug-mode
+// UNSUPPORTED: c++03
 
----------------
Quuxplusone wrote:
> Why is this test unsupported in C++03? Actually, what on earth is the point of this test?
> I'm fine shipping this commit as-is, as long as we get to the bottom of it in D100595.
> But I'd be happier with just `git rm`'ing this new file, because (unless I'm missing something) it's identical to db_iterators_2.pass.cpp except for the unneeded `UNSUPPORTED` line.
Thanks for pointing to this.
I didn't think much about the tests themselves while creating this patch, just extracted the second part of each test to a separate file and automatically marked them `UNSUPPORTED: c++03` because they were under `#if TEST_STD_VER >= 11`.
But you are right, we do not need `UNSUPPORTED: c++03` here (and for all the similar tests), the tests work fine with c++03 as well as with other supported versions of the standard. I guess this code was under `#if TEST_STD_VER >= 11` following min_allocator requirements, which needed c++11 or newer before D67675, but I didn't dig deeper to answer why min_allocator required c++ >= 11. I'll update the tests.

> Actually, what on earth is the point of this test?
This test unlike `db_iterators_2.pass.cpp` constructs a basic_string with a custom allocator. 
Basically, it follows the same pattern as other tests for containers (both normal and debug modes) where we have one test with default parameters and one with a custom allocator. So, I'd keep this test in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100592