[libcxx-commits] [PATCH] D110598: [libc++] P0980R1 (constexpr std::string)

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.access/at.pass.cpp:23
 template <class S>
-void
-test(S s, typename S::size_type pos)
-{
-    const S& cs = s;
-    if (pos < s.size())
-    {
-        assert(s.at(pos) == s[pos]);
-        assert(cs.at(pos) == cs[pos]);
-    }
+TEST_CONSTEXPR_CXX20 void test(S s, typename S::size_type pos) {
+  const S& cs = s;
----------------
As written, this test is not `TEST_CONSTEXPR_CXX20` because it requires exception-handling, right?
So you wrote the `constexpr_test` below.
I suggest that you either:
(Option 1) Move the non-throwing test cases into `test_constexpr()`, and make a main like this (minus my explanatory comments):
```
int main(int, char**) {
    test();  // run these parts only at runtime
    test_constexpr();  // run these parts at runtime, too...
#if TEST_STD_VER > 17
    static_assert(test_constexpr());  // ...but also at constexpr time, in C++20 and later.
#endif
}
```
or (Option 2, probably uglier) leave everything in `test`, and guard the throwing parts something like this:
```
// in test_macros.h
#if TEST_STD_VER > 17
#define TEST_NONCONSTEXPR_ONLY !std::is_constant_evaluated()
#else
#define TEST_NONCONSTEXPR_ONLY true
#endif

TEST_CONSTEXPR_CXX20 bool test() {
    if (pos < s.size()) {
        // run these parts both at runtime and at constexpr time
    }
#ifndef TEST_HAS_NO_EXCEPTIONS
    if (TEST_NONCONSTEXPR_ONLY) {
        // run these parts only at runtime
    }
#endif // TEST_HAS_NO_EXCEPTIONS
    return true;
}
int main(int, char**) {
    test();  // run these parts at runtime...
#if TEST_STD_VER > 17
    static_assert(test());  // ...and also at constexpr time, in C++20 and later.
#endif
}
```
I suspect that Option 1 may be the better option. Option 2 kinda confuses me, and I wrote it. :P


================
Comment at: libcxx/test/std/strings/basic.string/string.access/at.pass.cpp:58
+  {
+    typedef std::basic_string<char, std::char_traits<char>, min_allocator<char> > S;
     test(S(), 0);
----------------
This whitespace change is gratuitous; notice that we're inside a `TEST_STD_VER >= 11` block here. (Generally speaking, please don't listen to clang-format.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598