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

Mon Oct 11 08:31:01 PDT 2021

Quuxplusone added inline comments.

================
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;
+    }
----------------
philnik wrote:
> 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.
Now that D110994 exists, please "Add Parent Revision" to D110598, marking D110994 as a parent of D110598. (That's in the right sidebar of this page on Phabricator, https://reviews.llvm.org/D110598 .) That should make this PR's diff a lot smaller, while still allowing buildkite to build it properly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598