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.
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
Organize intrinsics const evaluability checks #61835
Organize intrinsics const evaluability checks #61835
Changes from all commits
0c2a54d
49fdb57
9f6ef64
21a5963
da6cc10
9484246
34951a9
5354f7f
c41c07b
54127de
07d1419
ab96b18
9328ee0
63cc638
e62acd4
3bc84dd
7ba0a57
ddaf56d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could we change this to match on a
Symbol
instead? This version is needlessly slow.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.
Also, @_nnethercote has been trying to remove queries that are easy to compute to improve performance. I think this could just be a function.Oh, this isn't actually exported in
providers
. My bad.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 wrapping intrinsics seem to be missing from this list. The errors mean the check is working! Awesome! Please add anything to this list that is needed to get the tests passing
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.
Isn't this
IsNotPromotable
? Shouldn't that check therustc_promotable
attribute instead of allowing allconst fn
?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.
Ah dang I might have mixed this up again with
IsNotImplicitlyPromotable
...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.
Note that this can't go wrong anymore, without triggering a mismatch with
promote_consts
' copy of the logic (although we should make sure this PR updates both properly), which would ICE.@ecstatic-morse and I will be removing the copy from
qualify_consts
in a week or two (maybe we just need to wait for beta to branch?), soIsNot(Implicitly) Promotable
is going away soon!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 just hope we get the intrinsic thing landed as part of this refactor, it's been 4 months or so.^^
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.
Oh yeah this PR can land at any time, and I think
promote_consts
even has the right code already, without another copy of the list of intrinsics.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.
Okay, but what about the new const checking pass? Does that still allow calling intrinsics?
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's a good question for @ecstatic-morse.
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 reserved a spot for these checks here. I think we just need to call the
is_const_intrinsic
from that spot? I dunno if this PR makes additional changes to the validation logic though.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 missed that this was baked into
in_const_fn
now (see below), I think this PR just needs to remove thatreturn
along with theFIXME
above it.