Skip to content

Check for unreachable in Select::finalize(Type) #6389

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

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Mar 8, 2024

Previously selects finalized with explicit types would never be marked
unreachable, even when they should have been.

@tlively tlively requested a review from kripken March 8, 2024 17:18
@tlively
Copy link
Member Author

tlively commented Mar 8, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite Graphite

@tlively
Copy link
Member Author

tlively commented Mar 8, 2024

Unfortunately I no longer have the test case the fuzzer produced, but hopefully this is straightforward enough to land anyway.

Previously selects finalized with explicit types would never be marked
unreachable, even when they should have been.
@tlively tlively force-pushed the select-finalize-fix branch from f77328a to 5e68848 Compare March 8, 2024 17:24
@tlively tlively enabled auto-merge (squash) March 8, 2024 17:32
void Select::finalize(Type type_) { type = type_; }
void Select::finalize(Type type_) {
assert(ifTrue && ifFalse);
if (ifTrue->type == Type::unreachable || ifFalse->type == Type::unreachable) {
Copy link
Member

@kripken kripken Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also include the condition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right 😅

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with that.

@tlively tlively merged commit df1044f into main Mar 8, 2024
@tlively tlively deleted the select-finalize-fix branch March 8, 2024 21:12
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants