[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/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:
> > 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)
I'm not sure what the `arc diff` workflow looks like. For my manual workflow, what you'd do is:
- `git fetch upstream main`
- `git checkout D110994 ; git rebase -i upstream/main`
- `git diff -U999 upstream/main D110994 > ~/foo.diff`
- https://reviews.llvm.org/D110994 "Update Diff", "Raw Diff From File", upload the `foo.diff` you just created
- `git checkout D110598 ; git rebase -i D110994`
- `git diff -U999 D110994 D110598 > ~/foo.diff`
- https://reviews.llvm.org/D110598 "Update Diff", "Raw Diff From File", upload the `foo.diff` you just created

So, no, I would say that "merging the two" is the exact opposite of what we're doing here. We're rebasing D110598 on top of D110994, so that the diff presented here for D110598 will //not// include any of D110994's changes (except that it will be based on top of them "by reference"). In git terms, the diff presented for D110994 will be `main...D110994`; the diff presented for D110598 will be `D110994...D110598`.

(P.S. I infer from your update comments that you've been doing a lot of "merge commits" (multi-parent git commits); you'll need to squash those away. Prefer a rebase-oriented workflow, where you keep scooping up your one atomic change and rebasing it //on top of// other people's unrelated changes in `main`, rather than incrementally modifying your change by merging new unrelated changes from `main` //into// it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598