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
Enable
loop
andwhile
in constants behind a feature flag #67216Enable
loop
andwhile
in constants behind a feature flag #67216Changes from all commits
8f3021b
57959b2
8f59902
ee233c0
99e132d
1122404
caa7c99
3325671
2add77d
a8e997c
a2a0774
b3aecd0
2b5ae1c
80581be
34ce0ba
598bed6
029725f
0f0bfc9
faa52d1
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.
This seems very fragile.
FalseUnwind
is supposed to be ignored (unless you care about unwind paths).What this code should be doing is a CFG cyclicity check.
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.
There is one in
check_consts
that will run even if#![feature(const_fn)]
is specified. I hope we can lose that feature gate in favor of fine-grained ones in the future, as well as mergecheck_consts
andqualify_min_const_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.
Do both run? That makes me happier, I guess, but then the
FalseUnwind
check here is not needed at all, is it?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.
check_consts
validation is always run.qualify_min_const_fn
is run when#![feature(const_fn)]
is not specified. My opinion is that this pass should be removed along with#![feature(const_fn)]
and any check that isn't already incheck_consts
given its own feature gate along with an entry incheck_consts/ops.rs
. I don't wanna spend political capital to spearhead this, however.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.
qualify_min_const_fn
was added as a hack back when the normal const-checking pass was such a mess that nobody was confident in its ability to rule out all bad code. It was always meant to be temporary.So, assuming const-check these days is well-structured enough that everyone is confident in its ability to do what it is supposed to do, I don't think you'll need to spend any political capital to get rid of it. Quite the contrary, at least I personally would be delighted to see it go. :)
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 would prefer if we could wait with removing
qualify_min_const_fn
. Not necessarily because the new const checker is messy, but because I don't understand the new code (which is based on data-flow) nearly as well as the old code. And as the lang team point-person for the const things, I would like to understand the thing that checks for stability wrt. const. So I'd appreciate if we don't do it right now and allow me some time to study the new stuff. (Also, is there an urgency to this?)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.
There's no urgency, there are some preliminary things that can be done like eliminating the
const_fn
feature gateThere 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.
You could just make the handling of
FalseUnwind
consistent, for now.It's not, and should not be mistaken for, a loop.
As the name says, it's a "false" (or "phantom") unwind edge. It's for situations where analyses which care about unwinding / cleanup blocks need to be conservative and assume unwinding can happen "out of the blue".
Everything else in const-checking ignores unwind edges (e.g. from calls) and associated cleanup blocks, because we completely bypass unwinding at compile-time.
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.
See #62274 for precedent.
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 looks like
FalseEdges
is also mishandled?