This patch makes std::rotate a constexpr. This way it can be used in other constexpr functions such as shift_left and shift_right.
mclow.lists EricWF ldionne
- Group Reviewers
- rG3ed89b51da38: [Take 2] [libc++] Make rotate a constexpr.
rG1ec02efee9b1: [libc++] Make rotate a constexpr.
This doesn't work because is_constant_evaluated is not a macro. We have the macro _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED that tells you whether std::is_constant_evaluated is supported.
Does this break the library when used with the debug mode? There must have been a reason why we added _LIBCPP_CONSTEXPR_IF_NODEBUG in the first place?
@EricWF Do we care about this? If not, it looks like now's the time to remove the debug mode (for something better) as we discussed.
I don't see the point of this #elif, can you explain?
This needs to be constexpr in C++20 and above only, since it's part of the API.
Please keep the previous formatting.
I think it should be the opposite: the tests fail in debug mode because this is no longer constexpr. I'll test it, though.
After C++17 the standard says some users of this function should be constexpr so, we have to support that. But before C++20, if _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED we probably want to take the fast path even if it means no constexpr. WDYT?
Your comment about loops made me think about generalized constexpr in C++14. Can you run the tests with your patch in C++11 and C++14 to make sure it works? I would expect failures due to the lack of generalized constexpr in C++11.
Can you try with _LIBCPP_DEBUG=2? It's a different debug level. I know it's kind of confusing.
I don't see why. Loops are allowed in constexpr since C++14. I guess we would need to use _LIBCPP_CONSTEXPR_AFTER_CXX14 then.
Essentially, the rule I'm trying to go for is: make private helpers constexpr as often as they can be, and make public APIs constexpr when the standard requires them to.
Hmm, interesting. Yeah, that makes sense, however it also means that we might be pessimizing the runtime version in C++20 just because the compiler doesn't have is_constant_evaluated. I'm not 100% sure what the best approach is, however I would tend to say: In C++20, we are conforming w.r.t. constexpr algorithms only when the compiler supports is_constant_evaluated, otherwise we are non-conforming in favour of performance. In other words, when is_constant_evaluated isn't provided, we would not properly provide constexpr in these algorithms, and we'd always use the fast-but-not-constexpr version. This would avoid silent pessimizations, but would reduce our conformance level with older compilers. I don't think it's a huge deal because libc++ should normally be used with a recent compiler anyway, but this merits some thinking.
If we go down that route, we could simply use __libcpp_is_constant_evaluated() instead of the logic you have here.
All tests pass :)
I didn't think to check, you're right loops in constexpr in C++14 work fine.
Great, I think this is a good idea.
My thinking here is that it's doubtful (especially at this point in time) that someone will be enabling C++20 on a compiler that isn't fairly recent.
But, this is actually a moot point because I just realized after P0202 we can use memmove in a constexpr context. So, we can remove __move_constexpr altogether.
Wait, this doesn't work at all because memmove isn't constexpr. You still need the _constexpr functions, I just meant that it was reasonable to not try to use their constexpr version when is_constant_evaluated() isn't supported by the compiler.
I would expect that your tests fail with the patch as it is (and if not there's something I don't understand, so please LMK).
You're right. I was looking at an earlier version of the paper where memmove was constexpr and then ran the tests in C++17 mode (when the constexpr bits weren't enabled) and thought, "this is great!"
Anyway, we could use __builtin_memmove after in Clang 8+ (which is constexpr).
This can't be constexpr in C++14 since it uses __rotate_forward, which is only marked as constexpr starting in C++17 and above.
I would remove && !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED) -- the test suite isn't supposed to have libc++ or compiler specific knowledge. If the test fails with some older compilers (e.g. clang 5), we can mark it as unsupported on that compiler.
If it can take one of the first two paths, then it can be constexpr. I think it makes sense to support it in C++14 when possible.
__builtin_is_constant_evalutated is actually pretty new. It was only added to clang in the last year IIRC. I think this will be a long list of compilers. The clang that ships on the latest macOS (Apple clang 11) doesn't even support it. It may be worth adding something to test_macros. WDYT?
You don't need #if _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED anymore if you're using __libcpp_is_constant_evaluated() -- IIUC.
Geez, you're right. I really glanced too quickly here.
My preference would still be to say // UNSUPPORTED: c++2a && apple-clang-11 (and the same for all unsupported compilers). The reason is that these UNSUPPORTED annotations are easy to get rid of as time goes by and we stop caring about older compilers. As such, it's easy-to-remove tech debt in the test suite.
On the other hand, if we engineer the test suite such that it can be run with compilers that don't support is_constant_evaluated(), we add complexity to the test suite itself, and removing it will require making sure that e.g. nobody's using the test suite with a compiler that doesn't support is_constant_evaluated(). Also, in theory, we should do the same for all features that involve compiler support, which isn't really scalable.
So.. my preference would be to be aggressive with compilers that don't support required features -- after all, libc++ is normally shipped alongside a matching Clang.
Yep, you're right.
Fair enough. I'll use UNSUPPORTED.
Why the C++2a part, though? I think I'll leave the TEST_STD_VER > 17 condition because that's what the standard says so, we shouldn't need the C++2a condition.
Also, I think we should support clang-8 and up. What are the corresponding apple-clang versions?
This almost LGTM. You can commit after fixing those comments and I'll adjust the markup for AppleClang as needed.
Same here, _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED isn't needed.
Same as above, please remove && !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED)
Same as above for && !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED).