[libcxx-commits] [PATCH] D115599: [libc++][NFC] Mark std::string functions as constexpr

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

I'd like to see this again with the `inline` keywords restored everywhere (and the keyword-order nit fixed).

However, given that CI is already green for this, personally I am happy to vote "land it" after that. We'll temporarily be in a state where our C++20 `string` methods are //marked// `constexpr` (as appropriate for C++20) but compile-fail when you try to use them at constexpr time... which is //philosophically// weird but has no //practical// downside that I can see, even if we stay in that state for months, even if we ship a release like that — it's totally a step forward from our status quo.



================
Comment at: libcxx/include/string:4408
 template<class _CharT, class _Traits, class _Allocator>
-inline
+_LIBCPP_CONSTEXPR_AFTER_CXX17
 basic_string<_CharT, _Traits, _Allocator>
----------------
Per https://reviews.llvm.org/D114136#inline-1089231 , let's not remove `inline` keywords. (Not in this specific PR, and not in this whole constexpr-string-related family of PRs in general). If we do that, we should do it separately from constexpr-string.

Aside: Personally I think it makes sense (and is harmless) to remove `inline` from forward declarations, as long as it is kept on the definitions; that makes work for the reviewer now (to make sure that every removed `inline` corresponds to a kept `inline` somewhere else in the file) but might save work in the future if done consistently. ...however, that sounds like a job for a separate PR to rationalize the //whole// codebase in that respect, not to do it piecemeal in this constexpr-related PR.


================
Comment at: libcxx/include/string:4435
 template<class _CharT, class _Traits, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
 basic_string<_CharT, _Traits, _Allocator>
----------------
Keyword-order nit throughout: `inline constexpr X`, not `constexpr inline X`.
```
$ git grep INLINE.*CONSTEXPR libcxx/include/ | wc -l
     774
$ git grep CONSTEXPR.*INLINE libcxx/include/ | wc -l
     130
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115599