Page MenuHomePhabricator

[libc++][NFC] Mark std::string functions as constexpr
AbandonedPublic

Authored by philnik on Dec 12 2021, 6:39 AM.

Details

Reviewers
ldionne
Quuxplusone
nilayvaish
Group Reviewers
Restricted Project
Summary

Mark std::string functions as constexpr. Behavioural changes and testing will be in D110598

Diff Detail

Event Timeline

philnik requested review of this revision.Dec 12 2021, 6:39 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2021, 6:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 393752.Dec 12 2021, 7:02 AM

Add inline back in non-template functions

Quuxplusone requested changes to this revision.Dec 12 2021, 8:05 AM

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
4390

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.

4417

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
This revision now requires changes to proceed.Dec 12 2021, 8:05 AM
ldionne accepted this revision.Dec 13 2021, 6:02 AM

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.

philnik updated this revision to Diff 394085.Dec 13 2021, 4:42 PM

re-added inline

philnik marked 2 inline comments as done.Dec 13 2021, 4:42 PM
philnik updated this revision to Diff 394086.Dec 13 2021, 4:46 PM

Missed one inline

nilayvaish accepted this revision.Dec 13 2021, 6:07 PM
ldionne requested changes to this revision.Dec 14 2021, 6:38 AM

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?

This revision now requires changes to proceed.Dec 14 2021, 6:38 AM

So you basically want this to contain all the tests and mark the functions as constexpr, and make the functional changes first?

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.

philnik updated this revision to Diff 395081.Dec 17 2021, 2:40 AM

Rebased onto main

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?

philnik abandoned this revision.Sat, Apr 30, 6:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Apr 30, 6:19 AM