[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/__memory/compressed_pair.h:61-62
 
-
-  _LIBCPP_INLINE_VISIBILITY reference __get() _NOEXCEPT { return __value_; }
-  _LIBCPP_INLINE_VISIBILITY
-  const_reference __get() const _NOEXCEPT { return __value_; }
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 reference __get() _NOEXCEPT { return __value_; }
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 const_reference __get() const _NOEXCEPT { return __value_; }
 
----------------
philnik wrote:
> Quuxplusone wrote:
> > For internal implementation details such as `__compressed_pair`, please be aggressive in adding `constexpr` (anywhere it's actually needed, of course). On these two lines, you can use `_LIBCPP_CONSTEXPR` instead of `_LIBCPP_CONSTEXPR_AFTER_CXX17`. The benefit is that on that far-future day where we drop support for C++03, we can just mechanically change all `_LIBCPP_CONSTEXPR` to `constexpr`; whereas we can't mechanically change `_LIBCPP_CONSTEXPR_AFTER_CXX17` to `constexpr` until we drop support for C++17!
> > 
> > For user-facing (public, standards-conforming) bits, of course we have to apply constexpr conservatively.
> I'm not sure what the problem is, but the test for C++11 fails (it think) because of the change to## _LIBCPP_CONSTEXPR##. C++03, C++14 and C++17 work fine. C++20 and C++2b most likely fail because constexpr string just doesn't work properly. Should I just change it to ##_LIBCPP_CONSTEXPR_AFTER_CXX11##? 
I suspect the problem is that in C++11, all `constexpr` member functions become implicitly `const`. (We did not really understand what `constexpr` should mean, until C++14. ;)) So for any //non-const// member functions, like `__get()` #1, you have to mark them as `_LIBCPP_CONSTEXPR_AFTER_CXX11`; but for //const// member functions, like `__get()` #2, you should still be able to use plain `_LIBCPP_CONSTEXPR`. Try that and see if I'm right. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110598