Mark std::string functions as constexpr. Behavioural changes and testing will be in D110598
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I'd like to see this again with the inline keywords restored everywhere (and the keyword-order nit fixed).
However, given that CI is already green for this, personally I am happy to vote "land it" after that. We'll temporarily be in a state where our C++20 string methods are marked constexpr (as appropriate for C++20) but compile-fail when you try to use them at constexpr time... which is philosophically weird but has no practical downside that I can see, even if we stay in that state for months, even if we ship a release like that — it's totally a step forward from our status quo.
libcxx/include/string | ||
---|---|---|
4408 | Per https://reviews.llvm.org/D114136#inline-1089231 , let's not remove inline keywords. (Not in this specific PR, and not in this whole constexpr-string-related family of PRs in general). If we do that, we should do it separately from constexpr-string. Aside: Personally I think it makes sense (and is harmless) to remove inline from forward declarations, as long as it is kept on the definitions; that makes work for the reviewer now (to make sure that every removed inline corresponds to a kept inline somewhere else in the file) but might save work in the future if done consistently. ...however, that sounds like a job for a separate PR to rationalize the whole codebase in that respect, not to do it piecemeal in this constexpr-related PR. | |
4435 | Keyword-order nit throughout: inline constexpr X, not constexpr inline X. $ git grep INLINE.*CONSTEXPR libcxx/include/ | wc -l 774 $ git grep CONSTEXPR.*INLINE libcxx/include/ | wc -l 130 |
LGTM with Arthur's suggestions. I want to press on restoring the inlines too until we understand whether that's the reason for the bloat we saw on our side.
I would actually like to hold off landing this patch until we can verify it with tests. We should be able to prepare everything to be constexpr friendly, and then in this patch we'd turn on the constexpr tests AND the constexpr annotations in std::string. Does that make sense to you?
So you basically want this to contain all the tests and mark the functions as constexpr, and make the functional changes first?
We could prepare the tests for constexpr-friendliness first, and then this would contain both the _LIBCPP_CONSTEXPR_AFTER_CXX17 and adding
#if TEST_STD_VER > 17 static_assert(tests()); #endif
in the test files. If that's doable, that would be ideal, but let me know if that's too much churn.
When I make the tests constexpr-friendly the CI will fail, because functions are marked constexpr which are not actually constant evaluatible. So having the tests in a different PR would mean the CI would fail in trunk or the warning has to be disabled in the PR en re-enabled here. I don't think either of these are viable. What should I do?
Per https://reviews.llvm.org/D114136#inline-1089231 , let's not remove inline keywords. (Not in this specific PR, and not in this whole constexpr-string-related family of PRs in general). If we do that, we should do it separately from constexpr-string.
Aside: Personally I think it makes sense (and is harmless) to remove inline from forward declarations, as long as it is kept on the definitions; that makes work for the reviewer now (to make sure that every removed inline corresponds to a kept inline somewhere else in the file) but might save work in the future if done consistently. ...however, that sounds like a job for a separate PR to rationalize the whole codebase in that respect, not to do it piecemeal in this constexpr-related PR.