Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Handle return calls in CodeFolding #6474

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Apr 5, 2024

Treat them the same as returns and test that they can be folded out of try-catch
blocks because they do not have throws effects.

@tlively
Copy link
Member Author

tlively commented Apr 5, 2024

@tlively tlively force-pushed the return-call-code-folding branch 2 times, most recently from 702e897 to e608ce4 Compare April 5, 2024 18:52
@tlively tlively changed the title [DRAFT] Handle return calls in CodeFolding Handle return calls in CodeFolding Apr 5, 2024
@tlively tlively requested a review from kripken April 5, 2024 18:53
@tlively
Copy link
Member Author

tlively commented Apr 5, 2024

cc @vouillon

@tlively tlively force-pushed the return-call-code-folding branch 2 times, most recently from 873166b to a72c996 Compare April 5, 2024 19:11
@tlively tlively force-pushed the return-call-effects branch from b4affd2 to 5ff3513 Compare April 5, 2024 19:24
@tlively tlively force-pushed the return-call-code-folding branch from a72c996 to e422e13 Compare April 5, 2024 19:24
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 % test questions

)
(unreachable)
)
)
Copy link
Member

Choose a reason for hiding this comment

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

did we not have a test for this before?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not that I was able to find for either of these tests. Except for the throws effect analysis, the implementation does not seem to special case try-catch at all, so it's reasonable that we wouldn't need tests for the success cases. But when I was trying to debug and figure out how to get this change working, it was useful to prove to myself locally that similar cases did already work correctly.

)
)
(i32.const 0)
)
Copy link
Member

Choose a reason for hiding this comment

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

same question.

@tlively tlively force-pushed the return-call-code-folding branch 3 times, most recently from e424ec1 to 6b593a0 Compare April 5, 2024 21:56
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.

Sounds good, makes sense.

@tlively tlively force-pushed the return-call-code-folding branch from 6b593a0 to 837aa77 Compare April 8, 2024 16:57
Base automatically changed from return-call-effects to return-call-ctor-eval April 8, 2024 17:02
Base automatically changed from return-call-ctor-eval to interpret-return-call April 8, 2024 17:03
Base automatically changed from interpret-return-call to return-call-inlining April 8, 2024 17:04
Base automatically changed from return-call-inlining to main April 8, 2024 17:50
Treat them the same as returns and test that they can be folded out of try-catch
blocks because they do not have throws effects.
@tlively tlively force-pushed the return-call-code-folding branch from 837aa77 to dc796d4 Compare April 8, 2024 18:28
@tlively tlively merged commit 102c363 into main Apr 8, 2024
26 checks passed
@tlively tlively deleted the return-call-code-folding branch April 8, 2024 18:52
@gkdn gkdn mentioned this pull request Aug 31, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants