This was discussed on Discord with the consensus that we should rename the macros.
Details
- Reviewers
ldionne Mordante var-const huixie90 fsb4000 avogelsgesang jloser - Group Reviewers
Restricted Project - Commits
- rG5146b57b403b: [libc++][NFC] Rename the constexpr macros
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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). |
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! |
libcxx/include/__config | ||
---|---|---|
827 | I am ok with the name in your patch. But have you considered |
libcxx/include/__config | ||
---|---|---|
827 | +1 for using the same naming style in our main code and our tests. |
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. |
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. |
- 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. |
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