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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 3 inline comments as done.
philnik added inline comments.


================
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
----------------
Quuxplusone wrote:
> 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`.
I think there is no need for a TODO, since the test, as you said yourself, tests an exception and therefore wouldn't work even if ##constexpr vector## was 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"));
----------------
Quuxplusone wrote:
> 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.
If you tell me how to disable the linter check before uploading I can do that. But I haven't seen any way around that, since it won't upload unless all lint checks work.


================
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;
+    }
----------------
Quuxplusone wrote:
> - 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?
Since it would probably be needed at some point anyways, I think it's best to open a new PR to make test_alloc_base and test_allocator constexpr. I'll try it in the next few days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598