[PATCH] D24941: [libc++][tests] Fixing some issues in unordered container tests

[Oleg Ranevskyy via llvm-commits]

iid_iunknown marked 5 inline comments as done.
iid_iunknown added a comment.

In https://reviews.llvm.org/D24941#553200, @EricWF wrote:

> Thanks for cleaning up our debug mode. I've added a couple of inline comments which need to be addressed.


Thank you for looking into this, Eric! Much appreciated.

Please find my inline answers below.

> If your interested there is much more work to do. For example moving all debug tests from `test/std` to `test/libcxx`, or fixing the debug tests to manually `#define _LIBCPP_DEBUG 1`.


Yes, I can do this once I am done with my other tasks. May we do this refactoring in a separate review request to avoid a relatively big commit?


================
Comment at: test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp:23
@@ -22,3 +22,3 @@
 #include <exception>
 #include <cstdlib>
 
----------------
EricWF wrote:
> Tests for debug mode should manually enable it, instead of requiring it already be enabled. If you want to do more cleanup please change this test to `#define _LIBCPP_DEBUG 1` at the start of the file, and then remove the redundant empty main.
Done. Several other tests under test/libcxx have also been fixed.

================
Comment at: test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp:31
@@ -30,3 +30,3 @@
     typedef std::unordered_map<int, std::string> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    C c({{1, std::string()}});
+    C::local_iterator i = c.begin(c.bucket(1));
----------------
EricWF wrote:
> This uses C++11 features but is required to work in C++03. Please make sure these changes don't break C++03 tests.
> 
> Side note: Wouldn't removing the second increment be simpler than changing the size? 
> This uses C++11 features but is required to work in C++03.
Thanks for catching this! Fixed.

> Wouldn't removing the second increment be simpler than changing the size?
My thought was that these 2 increments were added intentinally to test successful and unsuccessful increment. On the other hand, other tests already check successful increments and retesting them here is superfluous. Removing element insertion and one increment according to your advice.

================
Comment at: test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp:35
@@ -34,3 +34,3 @@
     ++i;
     assert(false);
     }
----------------
EricWF wrote:
> All of the code below this is logically dead. Please either (A) remove it, or (B) define `_LIBCPP_ASSERT` so it doesn't exit when invoked. For example:
> 
> ```
> #define _LIBCPP_ASSERT(p, m) ((p) ? (void)0 : throw 0)
> int main()
> {
>     typedef std::unordered_map<int, std::string> C;
>     C c(1);
>     C::local_iterator i = c.begin(0);
>     try {
>       ++i;
>       assert(false);
>     } catch(int) {}
>     // More tests can be written below
> }
> ```
Agree. We build all the libc++ tests into a single executable binary to test on our system. We also have custom assert, exit() / abort() and _LIBCPP_ASSERT (similar to the one you suggested) so that test failures do not stop execution. Thus our testing env is not affected, which is why I didn't fix this. The usual test workflow does suffer from this however. Fixing these and some other tests according to your advice.


https://reviews.llvm.org/D24941