-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Support sorting consteval-only ranges #134623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
frederick-vs-ja
wants to merge
10
commits into
llvm:main
Choose a base branch
from
frederick-vs-ja:sort-consteval-iter
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d4f2563
[libc++] Support sorting consteval-only ranges
frederick-vs-ja 9625fe9
Create `test_consteval_iterators.h` and include it
frederick-vs-ja 421e46e
Remove workaround for other standard libraries
frederick-vs-ja 3aa6f3e
Merge branch 'main' into sort-consteval-iter
frederick-vs-ja 2ce3060
Merge branch 'main' into sort-consteval-iter
frederick-vs-ja 5d68c59
Merge branch 'main' into sort-consteval-iter
frederick-vs-ja 90d2ac2
Avoid violating precondition of `__bit_log2`
frederick-vs-ja c189d67
Fix typo
frederick-vs-ja 6cd3e63
Eliminate `__sort` branches and fix uses of `_AlgPolicy`
frederick-vs-ja f06b9cc
Merge branch 'main' into sort-consteval-iter
frederick-vs-ja File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. How is this changing anything compared to the general
__libcpp_is_constant_evaluated
? IIUC you claim this has got something to do with instantiating an undefined template, but this is instantiating__sort
exactly the same as the old code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guarded this with
_LIBCPP_STD_VER >= 20
becauseranges::less
is used in__sort
and I wanted the same operations to be performed. And given_LIBCPP_STD_VER >= 20
is required, IMO its a bit better to usestd::is_constant_evaluated
. (Or even C++23if consteval
?)No, the changes in
__sort_dispatch
didn't change instantiation of__sort
. I just attempted to add constexpr-friend branches to them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please revert these changes. It's not at all obvious to me what changes actually fix anything (and FWIW I'm not convinced these changes are a good idea).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think the changes here "fix" an ironic inconsistency.
Some functions needs to be
constexpr
to make instantiationsort
involving consteval-only operations well-formed, while without these changes they are not even called in constant evaluation. If we keep these functions not called in constant evaluation, maybe someone will get confused and attempt to removeconstexpr
in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. How does it make a difference whether some functions that aren't constexpr are instantiated or not? How does it not make a difference when they can't be instantiated because they're not defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the error is not about undefined functions. It is about functions templates that are defined but not constexpr (i.e.
__introsort
etc. in the status quo).The expression
__last - __first
inside__introsort
can't be a constant expression because__first
and__last
are function parameters. When theoperator-
for the iterator is aconsteval
function and__introsort
is notconstexpr
,__last - __first
would be required to be a constant expression, which makes instantiation of__introsort
ill-formed.Marking
__introsort
constexpr
makes the__introsort
specialization gets immediate-escalated and becomes an immediate function, and thus its instantiation can be well-formed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is quite unfortunate. IMO it shouldn't make a difference whether we define a function or not. Anyways, can't we just replace the
if (__libcpp_is_constant_evaluated())
withif consteval
? That would make the diff significantly smaller.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work, because instantiation of
__introsort
is still triggered. AFAIK__introsort
needs to be instantiated for most iterator types, and there's no mechanism to avoid instantiation of some function only during constant evaluation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, IMO this is CWG issue territory. It can't be that
consteval
goes this viral. This basically means that we can't have non-constexpr
code inconstexpr
functions, which is simply not acceptable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crux is the combination of non-constant
consteval
operations and non-constexpr
functions. Currently non-constexpr
functions, along with non-template non-lambda non-defaultedconstexpr
functions, are never immediate-escalated. Perhaps it's also possible to escalate some non-constexpr
functions to "error-on-call-only" things.However, is it a bad thing to use
__introsort
in constant evaluation? If not, it seems plausible to mark it and its depended functions_LIBCPP_CONSTEXPR_SINCE_CXX20
.