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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 4 inline comments as done.
philnik 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;
+    }
----------------
Quuxplusone wrote:
> 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.
Do I have to merge the two and upload that? (and I think "a lot" is too strong of a statement here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598