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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:1466
 #if _LIBCPP_STD_VER > 17
     _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
     bool starts_with(__self_view __sv) const _NOEXCEPT
----------------
miscco wrote:
> philnik wrote:
> > miscco wrote:
> > > Quuxplusone wrote:
> > > > These existing C++20-only methods could all be `_LIBCPP_CONSTEXPR` instead of `_LIBCPP_CONSTEXPR_AFTER_CXX11`; but I think I'll open a new separate PR for that, so never mind.
> > > I am pretty sure they should be plain `constexpr`
> > Should I just include the fix in this PR, since it isn't important that it's fixed in trunk and is related to making string constexpr? 
> Yeah, I would not wait on this. In the end it is something the maintainers should decide
No, I'll do this in D110637.
I think that the //ideal// situation for this PR would be if we can get it down to just the minimal patch needed for P0980, with everything else (scope-creep, any prerequisite refactorings, etc.) pulled out into earlier PRs. If @miscco is right that your "always `__is_long()`" invariant is unnecessary, then it's //possible// this PR could be stripped down to just a huge tedious set of `_LIBCPP_CONSTEXPR_AFTER_CXX17` additions, and the tests.

Btw, for the tests, libc++'s general approach is to write a test function like
```
TEST_CONSTEXPR_CXX20 bool test() {
    std::string s;
    assert(s.whatever());
    return true;
}
int main() {
    test();  // this is the runtime test coverage
#if TEST_STD_VER > 17
    static_assert(test());  // this is the additional compile-time test coverage
#endif
}
```
I haven't looked at the existing string tests, but I assume this is going to be a //massive// code churn there as well. I might even think about whether it makes sense to pull out the test-suite refactoring into a separate, prerequisite PR; and then this PR's test-suite changes could be limited to tediously changing hundreds of instances of `#if 0` into `#if TEST_STD_VER > 17`.


================
Comment at: libcxx/test/std/input.output/iostream.format/quoted.manip/quoted_traits.verify.cpp:25
 template <class charT>
-struct test_traits {
-    typedef charT char_type;
+struct test_traits : std::char_traits<char> {
+  typedef charT char_type;
----------------
philnik wrote:
> Quuxplusone wrote:
> > I recommend not inheriting from standard types in general. What causes this change to seem like a good idea? Is `test_traits` missing some members that `std::quoted` needs? Which ones?
>  ##basic_string()## needs ##assign## now, because might call ##__init(size_type, value_type)##. Should I just copy/paste it from ##std::char_traits##?
I would say yes, just copy and paste it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598