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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__string:302-306
+    if (__n == 0) return __s1;
+    _CharT* __alloc = new _CharT[__n];
+    __copy_constexpr(__alloc, __s2, __n);
+    __copy_constexpr(__s1, static_cast<const _CharT*>(__alloc), __n);
+    delete[] __alloc;
----------------
For sanity's sake, these `foo_constexpr` functions should all begin with `_LIBCPP_ASSERT(__libcpp_is_constant_evaluated());` — we don't expect them ever to be called at runtime. (In '20 we could make them `consteval`; but I think the assertion is the better, more explicit, solution, even in '20.) If this `new`/`delete` dance ever got codegenned into runtime code, that would be horrible. We could add the assert in a separate PR, or just roll it into this one.

`__copy_constexpr(...` should be `_VSTD::__copy_constexpr(...` for ADL-proofing.


================
Comment at: libcxx/include/string:625
     _LIBCPP_NORETURN void __throw_length_error() const;
-    _LIBCPP_NORETURN void __throw_out_of_range() const;
+    _LIBCPP_NORETURN  void __throw_out_of_range() const;
 };
----------------
Bogus extra space here.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/clear.pass.cpp:60-62
+
+  test();
+
----------------
Whitespace nit: I'd prefer to remove these blank lines (60 and 62), in that they don't really help anything. Ditto throughout.
However, excellent transformations of all these tests! This is exactly what I had in mind, and I'm glad it's mostly turning out to be easy. (Assuming the transformed tests do still pass, of course.)


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr.pass.cpp:112-114
+#    if __cpp_lib_constexpr_vector >= 201907L
+TEST_CONSTEXPR_CXX20
+#    endif
----------------
IIUC, you're preemptively changing this test so that once `constexpr vector` is supported, the test suite will magically pick it up and start testing it. This seems gratuitous; I'd rather see `constexpr vector` test-suite changes happen at the same time as their matching code changes, so that we can see from looking at the PR that "yes, we thought about test coverage." Having the test suite magically adapt to test only what works is, like, the opposite of the point of a test suite. ;)
Also, preemptive changes like this are potentially incorrect. In this case, I'm pretty sure that this test //cannot// be constexpr, because it relies on throw/catch. So it passes today because `constexpr vector` isn't implemented; but as soon as we implement that code change, we'll find that this test starts failing for unrelated reasons.

I suggest replacing this diff with more like `// TODO: update when P1004 constexpr vector is implemented`.


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.ptr.pass.cpp:61
+  assert(!sNot.contains("abcde"));
+  assert(sNot.contains("xyz"));
+  assert(!sNot.contains("zyx"));
----------------
In general, I wish you weren't clang-formatting these files as you change them. In //most// cases, Phabricator can deal sensibly with the gratuitous whitespace changes, but in some cases (like this file), the gratuitous whitespace diffs are actively distracting. Remember, //just don't use clang-format// and you won't have problems.


================
Comment at: libcxx/test/support/test_allocator.h:89-93
+    TEST_CONSTEXPR_CXX20
+    test_allocator() TEST_NOEXCEPT : data_(0), id_(0) {
+        if (!TEST_IS_CONSTANT_EVALUATED)
+            ++count;
+    }
----------------
- Since this is not public-facing, let's try to use the "most/oldest" constexpr possible. E.g. this one can be `TEST_CONSTEXPR_CXX14`, and some of the others can probably be just `constexpr`.
- I'm not thrilled with the general idea here of "skip some of the effects when constexpr-evaluated." It would be much cleaner (but possibly make a //huge// PR) to either (1) leave `test_allocator` as non-constexpr-friendly, and skip those tests in constexpr mode; or (2) refactor `test_allocator`'s statistics collection to be constexpr-friendly; basically it would hold a `test_allocator_statistics *stats_` in addition to its `data_` and `id_`, and we'd make sure that all the tests that care about the stats always initialize their `test_allocator` instances with some `&localstats`, and then look at `localstats.count` rather than trying to look at the global `test_allocator_base::count` etc.
What's your opinion on those ideas?


================
Comment at: libcxx/test/support/test_macros.h:13
 
+#include <version>
+
----------------
Do we need the `__has_include` dance here, for the benefit of non-libc++ libraries? (The answer is above my pay grade.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598