[libcxx-commits] [PATCH] D120951: [libc++] Prepare string tests for constexpr

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM%comments, and all the comments are pretty easy to deal with, I think.



================
Comment at: libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:24
 
-    alloc_imp() : active(true) {}
+    TEST_CONSTEXPR_CXX14 alloc_imp() : active(true) {}
 
----------------
Prefer `TEST_CONSTEXPR` (unless I'm missing a reason this doesn't work in C++11).


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:27
     template <class T>
-    T* allocate(std::size_t n)
+    TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n)
     {
----------------
This either calls `std::malloc` or throws. I don't think it's safely constexpr-qualifiable even in C++20. Does it need to be?
Ditto `deallocate`.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:41-67
 template <class T>
 struct poca_alloc {
     typedef T value_type;
     typedef std::true_type propagate_on_container_copy_assignment;
 
     alloc_imp *imp;
 
----------------
Pre-existing: It'd be awesome to `s/poca/pocca/` and move the operators into hidden friends. But in a separate PR for sure.

You put the `poca_alloc` codepath on lines 109-127 under `!TEST_IS_CONSTANT_EVALUATED`; is it possible that these constexpr-qualifications are unnecessary?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr.pass.cpp:233-235
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  test2583();
+#endif
----------------
This is missing an `>= 11` guard; but in fact I think what you should do here is //remove// this guard, and move the guards around `test[_lwg]2583` //inside// the function body. Also some drive-by cleanup:
```
void test_lwg2583()
{
#if TEST_STD_VER >= 11 && defined(TEST_HAS_NO_EXCEPTIONS)
    typedef std::basic_string<char, std::char_traits<char>, test_allocator<char>> StringA;
    std::vector<StringA, std::scoped_allocator_adaptor<test_allocator<StringA>>> vs;
    StringA s{"1234"};
    vs.emplace_back(s, 2);
    try {
        vs.emplace_back(s, 5);
        assert(false);
    } catch (const std::out_of_range&) { }
#endif
}
```


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_append/iterator.pass.cpp:218-233
+  if (!TEST_IS_CONSTANT_EVALUATED) { // regression-test appending to self in sneaky ways
     std::string s_short = "hello";
     std::string s_long = "Lorem ipsum dolor sit amet, consectetur/";
     std::string s_othertype = "hello";
     const unsigned char *first = reinterpret_cast<const unsigned char*>(s_othertype.data());
     std::string s_sneaky = "hello";
 
----------------
Here, I assume the only problem is with `first`'s `reinterpret_cast`; the other 3 calls to `test` are OK, right? Consider refactoring to pull out `first` into its own code block (at the expense of some duplication, I assume, but that's fine).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120951