Skip to content

[Outlining] Add Try/Catch/CatchAll #7472

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 21 commits into from
Apr 29, 2025
Merged

[Outlining] Add Try/Catch/CatchAll #7472

merged 21 commits into from
Apr 29, 2025

Conversation

ashleynh
Copy link
Collaborator

@ashleynh ashleynh commented Apr 9, 2025

Supports try/catch/catchAll in the stringify of the module. Its contents can now be outlined.

Base automatically changed from logging to main April 9, 2025 18:53
@ashleynh ashleynh changed the title [Outlining] Try/Catch/CatchAll [Outlining] Add Try/Catch/CatchAll Apr 15, 2025
@ashleynh ashleynh requested a review from tlively April 24, 2025 18:53
@ashleynh ashleynh marked this pull request as ready for review April 24, 2025 18:53
Comment on lines 141 to 144
// We preserve the name of the tryy because IRBuilder expects
// visitTryStart() to be called on an empty Try, during the normal case of
// parsing.
auto name = curr->tryy->name;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We preserve the name of the tryy because IRBuilder expects
// visitTryStart() to be called on an empty Try, during the normal case of
// parsing.
auto name = curr->tryy->name;
// We preserve the name of the tryy because IRBuilder expects
// visitTryStart() to be called on an empty Try, during the normal case of
// parsing. TODO: Fix this.
auto name = curr->tryy->name;

Comment on lines 402 to 403
makeTry(Try* tryy, Name originalLabel, Type inputType, Index index) {
return ScopeCtx(TryScope{tryy, originalLabel, index}, inputType);
Copy link
Member

Choose a reason for hiding this comment

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

It would never make sense to create a Try scope with any index other than 0.

Suggested change
makeTry(Try* tryy, Name originalLabel, Type inputType, Index index) {
return ScopeCtx(TryScope{tryy, originalLabel, index}, inputType);
makeTry(Try* tryy, Name originalLabel, Type inputType) {
return ScopeCtx(TryScope{tryy, originalLabel, 0}, inputType);

Comment on lines 405 to 406
static ScopeCtx makeCatch(ScopeCtx&& scope, Try* tryy, Index index) {
scope.scope = CatchScope{tryy, scope.getOriginalLabel(), index};
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the index parameter here and do the increment automatically, since it never makes sense to create a Catch (or CatchAll) scope with any index other than one more than the previous index.

Comment on lines 951 to 962
// Indexes are managed manually to support Outlining.
// Its prepopulated try catchBodies and catchTags vectors
// cannot be appended to, as in the case of the empty try
// used during parsing.
if (tryy->catchBodies.size() < index) {
tryy->catchBodies.resize(tryy->catchBodies.size() + 1);
}
// The first time visitCatch is called: the body of the
// try is set and catchBodies is not appended to, but the tag
// for the following catch is appended. So, catchTags uses
// index as-is, but catchBodies uses index-1.
tryy->catchBodies[index - 1] = *expr;
Copy link
Member

Choose a reason for hiding this comment

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

Let's put this all in a helper function since it's repeated three times.

@ashleynh ashleynh requested a review from tlively April 29, 2025 20:06
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashleynh ashleynh merged commit d48529d into main Apr 29, 2025
14 checks passed
@ashleynh ashleynh deleted the outlining_try branch April 29, 2025 20:41
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