[libcxx-commits] [PATCH] D131498: [libc++][NFC] Rename the constexpr macros

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM, but I'd like to use `SINCE`, and I think the side discussion of what to do with internal helpers is worth figuring out without necessarily taking any action in this patch.



================
Comment at: libcxx/include/__config:835
 #  if _LIBCPP_STD_VER > 11
-#    define _LIBCPP_CONSTEXPR_AFTER_CXX11 constexpr
+#    define _LIBCPP_CONSTEXPR_CXX14 constexpr
 #  else
----------------
Mordante wrote:
> philnik wrote:
> > jloser wrote:
> > > huixie90 wrote:
> > > > I am ok with the name in your patch. But have you considered
> > > > `_LIBCPP_CONSTEXPR_SINCE_CXX14`
> > > > The reason why I like this name is because in the synopsis it is exact these words "// constexpr since C++14"
> > > I quite like the naming convention of using "since", e.g. `_LIBCPP_CONSTEXPR_SINCE_CXX14` as you proposed, @huixie90. I'm also not opposed to @philnik current proposed rename as I do like it better than the status quo on top of tree.
> > I have considered the name, but I think that `_LIBCPP_CONSTEXPR_SINCE_CXXab` is unnecessarily verbose. We already use `TEST_CONSTEXPR_CXXab` in the tests and AFAIK nobody ever complained about it. @ldionne also proposed `_LIBCPP_CONSTEXPR_IN_CXXab`, but there the same applies.
> +1 for using the same naming style in our main code and our tests.
My preference is also `_LIBCPP_CONSTEXPR_SINCE_CXX14`. If we want full consistency, I would actually rename the test macros to `TEST_CONSTEXPR_SINCE_CXX14`. I think that `SINCE` expresses the intent of the macro much better, i.e. "this API is constexpr since C++xy". Without `SINCE`, one could imagine that this instead represents e.g. the `constexpr` capabilities of the compiler, which is not the case (we assume those capabilities, instead). So yeah, big +1 for using `SINCE`.

In fact, I think we really have two macros here. Once of them is `_LIBCPP_CONSTEXPR_SINCE_CXXYZ`, aimed towards our API, and the other one is `_LIBCPP_CXXYZ_CONSTEXPR_SUPPORTED`, aimed for implementation detail functions that essentially want to be `constexpr` as early as the language supports it. I am not sure it makes sense to introduce that macro, though, but it illustrates why I like the `SINCE` variant better.


================
Comment at: libcxx/include/__memory/compressed_pair.h:44
   template <class... _Args, size_t... _Indices>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_CXX17
   explicit __compressed_pair_elem(piecewise_construct_t, tuple<_Args...> __args, __tuple_indices<_Indices...>)
----------------
I think this should work? In fact we could just use `constexpr`, maybe.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:625
                           __allocator_has_trivial_move_construct<_Alloc, _Type>::value> >
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 _Iter2
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_CXX20 _Iter2
 __uninitialized_allocator_move_if_noexcept(_Alloc&, _Iter1 __first1, _Iter1 __last1, _Iter2 __first2) {
----------------
For example, is there a reason why this is only constexpr in C++20? Maybe it should be marked as constexpr whenever we have support for C++14 generalized constexpr?


================
Comment at: libcxx/test/libcxx/nasty_macros.compile.pass.cpp:146
+#define _LIBCPP_CONSTEXPR_AFTER_CXX14 NASTY_MACRO
+#define _LIBCPP_CONSTEXPR_AFTER_CXX17 NASTY_MACRO
+
----------------
Mordante wrote:
> Thanks!
I don't think this is necessary. I understand the goal of catching non-rebased patches, however such patches are going to fail with unknown identifier `_LIBCPP_CONSTEXPR_AFTER_CXX14` anyway if someone forgets to use the new macro names. So I don't think this is going to catch anything that we wouldn't otherwise catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131498