Page MenuHomePhabricator

[libc++][NFC] Rename the constexpr macros
ClosedPublic

Authored by philnik on Aug 9 2022, 7:59 AM.

Details

Summary

This was discussed on Discord with the consensus that we should rename the macros.

Diff Detail

Event Timeline

philnik created this revision.Aug 9 2022, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 7:59 AM
philnik requested review of this revision.Aug 9 2022, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 7:59 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
avogelsgesang accepted this revision.Aug 10 2022, 1:17 AM

LGTM, thanks!

I didn't actually look through all the files, I only looked at a random sample. I trust that you made the changes using sed or some other automation, and that the test cases would catch any functional regressions

libcxx/.clang-format
15–18

not sure what the outcome of the Discord discussion was (somehow I can't find the channel anymore?), but personally I would prefer to also change _LIBCPP_CONSTEXPR_AFTER_CXX20 to _LIBCPP_CONSTEXPR_CXX23

Please don't let this stop you from merging your change though. It is a clear improvement compared to the previous state

philnik added inline comments.Aug 10 2022, 5:17 AM
libcxx/.clang-format
15–18

Oops. We don't actually have a _LIBCPP_CONSTEXPR_AFTER_CXX20 yet. Looks like I accidentally added it to the clang-format. I'll rename it when I commit the patch (assuming nobody requests further changes).

Mordante accepted this revision as: Mordante.Aug 10 2022, 8:21 AM

Thanks for working on this, it's great to have everything consistent in one go!
I have spot checked the changes, but I assume you used a tool to do the changes automatically.
LGTM, but I think it's good to get a few more explicit approvals so leave the final approval to somebody else.

libcxx/.clang-format
15–18

Mainly curious can this be a regex?

libcxx/test/libcxx/nasty_macros.compile.pass.cpp
146 ↗(On Diff #451153)

Thanks!

philnik marked 2 inline comments as done.Aug 10 2022, 8:39 AM
philnik added inline comments.
libcxx/.clang-format
15–18

The clang-format docs say that AttributeMacros is a List of Strings. So I don't think so. I've made a feature request here

huixie90 added inline comments.Aug 11 2022, 3:38 PM
libcxx/include/__config
827

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"

jloser added inline comments.Aug 12 2022, 6:04 AM
libcxx/include/__config
827

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.

philnik marked an inline comment as done.Aug 12 2022, 6:14 AM
philnik added inline comments.
libcxx/include/__config
827

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.

jloser accepted this revision.Aug 12 2022, 6:18 AM
Mordante added inline comments.Aug 13 2022, 5:10 AM
libcxx/include/__config
827

+1 for using the same naming style in our main code and our tests.

ldionne accepted this revision.Aug 18 2022, 10:16 AM

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.

libcxx/include/__config
827

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.

libcxx/include/__memory/compressed_pair.h
45

I think this should work? In fact we could just use constexpr, maybe.

libcxx/include/__memory/uninitialized_algorithms.h
626

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?

libcxx/test/libcxx/nasty_macros.compile.pass.cpp
146 ↗(On Diff #451153)

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.

This revision is now accepted and ready to land.Aug 18 2022, 10:16 AM
var-const accepted this revision.Aug 18 2022, 2:38 PM
var-const added inline comments.
libcxx/include/__memory/uninitialized_algorithms.h
626

General question -- for internal functions, should we make them constexpr in the lowest language mode possible, or just in the language mode where they are actually used? I'm slightly concerned about overconstraining the implementation, though we can probably always work around that if necessary.

philnik updated this revision to Diff 453948.Aug 19 2022, 4:11 AM
philnik marked 5 inline comments as done.
  • Use _SINCE_ style

I'll land this as soon as possible, since it will conflict with a lot of patches

libcxx/include/__memory/uninitialized_algorithms.h
626

We normally use what works, i.e. the lowest language mode feasible. Don't try to make some function constexpr in C++11 just because it's technically possible. But you might as well do it when the function would work in C++11 constexpr.

This revision was automatically updated to reflect the committed changes.