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

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

Quuxplusone added a comment.

Please update the PR summary (i.e. the proposed commit message and general info about the patch). Particularly, I'd like to see some paragraph-form text explaining the general approach. I'm sure it's been explained and discussed several times, e.g. on Discord, probably a couple times in this PR's comments... but IIUC the details have also changed over time. An up-to-date description belongs in the PR summary, and ultimately in the commit message. It would even be appropriate to add something to `libcxx/docs/DesignDocs/`. All of this will benefit the next person to maintain this and/or to implement constexpr vector or constexpr whatever-they-do-in-C++23.

It would help if this were split into relatively orthogonal chunks, most of which could be "NFC." For example:

- The `test_iterators.h` diff, which is just adding constexpr to one type, can be landed today AFAIC.
- The `test_macros.h` diff is probably unnecessary?
- A huge number of diffs in `<string>` are just adding the word `_LIBCPP_CONSTEXPR_AFTER_CXX17`; since this is all template code where the compiler shouldn't complain about incorrect constexpr keywords, can we land those as a separate patch, before the behavioral changes?
- Vice versa, can we land the behavioral changes first and then the `_LIBCPP_CONSTEXPR_AFTER_CXX17` markings second?
- Several new member functions: `__begin_lifetime`, `__finish_replace`, `__free_memory_constexpr`, `__insert_not_in_range`. Can we introduce those first (changing the "shape" of the control flow), and then in a second step change their behavior to include the constexpr-friendly codepaths?
- The constexpr-ified tests do need to be landed in the same commit as the behavior they test... but //for review purposes// they are orthogonal/distracting.
- Even if we //land// this PR as a monolith, it would still be vastly easier to //review// in small pieces like that. If you're trying to review the control flow, all the `_LIBCPP_CONSTEXPR_AFTER_CXX17` markings are distracting; and vice versa.

I don't find Phab's interface very convenient for splitting up patches in these ways. You //can// make a bunch of Phab PRs and link them together with dependencies, but really it might be easier to just edit the issue summary to include [the design info, plus] a link to a branch on your own GitHub (I mean a URL like https://github.com/Quuxplusone/llvm-project/compare/main...trivially-relocatable ) and say "Hey reviewers, if you prefer to see this PR broken down into its orthogonal component parts, take a look at this series of commits in GitHub."



================
Comment at: libcxx/include/string:4256-4264
     if (size() > capacity())
         return false;
-    if (capacity() < __min_cap - 1)
+    if (capacity() < __min_cap - 1 && !__libcpp_is_constant_evaluated())
         return false;
     if (data() == nullptr)
         return false;
     if (data()[size()] != value_type())
----------------
The PR summary (and possibly a design doc) should explain why the class invariants need to change in constexpr-land.
But aside from that, I find this diff hard to reason about. After some fiddling with it, I think the cleanest version introduces the concept of "SSO capacity," like this:
```
    size_type __sso_capacity = __libcpp_is_constant_evaluated() ? 0 : (__min_cap - 1);
    if (size() > capacity())
        return false;
    if (capacity() < __sso_capacity)
        return false;
    if (data() == nullptr)
        return false;
    if (data()[size()] != value_type())
        return false;
    return true;
```
IIUC, you use this calculation several places in this PR. Consider introducing a function `__sso_capacity()` for this purpose... except that at first glance I'm worried about any function whose return value changes depending on constexprness. I'm not sure that such a function is safe to use. So maybe pulling it out into a function would be very bad, I'm not sure.


================
Comment at: libcxx/include/string:1818-1820
+        if (__libcpp_is_constant_evaluated())
+                for (size_t __i {}; __i < __count; ++__i)
+                    new (&__p[__i]) value_type();
----------------
Quuxplusone wrote:
> - Braces around multi-line branches, please (in this case, around the `if`)
> - `size_t __i = 0` not `size_t __i {}`
> - `::new` not `new` (ADL-proofing)
> - I question the usefulness of `consteval` here; seems like a simple `constexpr` would do; but at least if you //keep// it `consteval`, then you certainly don't need to branch on `__libcpp_is_constant_evaluated()` because that will always be true. Actually, I think you call this on line 1956 in a non-constexpr context, so `consteval` should not compile, right?
`_VSTD::construct_at(_VSTD::addressof(__p[__i]), value_type()));` for ADL.
On line 1785, please linebreak before `void`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598