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
const evaluatable: improve
TooGeneric
handling #77303const evaluatable: improve
TooGeneric
handling #77303Changes from all commits
a4783de
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.
is there any particular reason we need to walk this in tree order and not just visit all nodes?
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.
So for subtrees that would be incorrect, as they can contain nodes from other subtrees.
I think if we always use a method like this for a complete tree that would be fine. Kinda afraid that we accidentially incluse unused nodes though 🤔
About unused nodes, do we want to emit an error if we encounter them during
AbstractConst
building.Let me test how this deals with things like
N + 1; 3
rn.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.
results in
which is currently allowed. Do we want to forbid unused nodes in abstract const building?
edit: I think we do, will open a PR for that (once this is merged as I want to reuse
walk_abstract_const
for that)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.
any kind of recursion should probably use
ensure_sufficient_stack
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.
Will we ever get really deep trees here?
so something like
1 + 1 + 1 + 1 + 1 ....
is probably the deepest we can go,I think we catch errors here somewhere else first.
I personally would like to wait on an actual bug report here, as
ensure_sufficient_stack
has a small perf impactThere 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.
It may occur in generated code somewhere, and if the rest of the compiler code is already recursion depth resistant we can hit this here. Once this is merged and in nightly I'll try to build a test that hits 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.
I don't know anything about this part of the compiler...
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.
👍
yeah, then let's have @varkor or @eddyb review that part