Skip to content
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

Lower IfLet expressions #1218

Merged
merged 1 commit into from
May 10, 2022
Merged

Conversation

antego
Copy link
Contributor

@antego antego commented May 5, 2022

This PR addresses #1177. This change implements the "hir-lowering" part of the plan outlined here #1177 (comment).

@philberty philberty self-requested a review May 5, 2022 13:10
@philberty
Copy link
Member

Would you like a review @antego or would you still like to do more here?

@antego
Copy link
Contributor Author

antego commented May 6, 2022

Ideally I'd like to make the compilation work. But now I'm not sure since there are possibly too many changes for one PR judging by this #1177 (comment)

@CohenArthur
Copy link
Member

@antego all the steps mentioned in the issue should be done in multiple PRs imo. It's a good idea to have a PR dedicated to lowering, one to name resolution, one to type checking, one to backend compilation etc.

@antego antego force-pushed the lower-iflet-expressions branch 2 times, most recently from f583322 to e125119 Compare May 7, 2022 00:47
@antego antego marked this pull request as ready for review May 7, 2022 00:47
@antego
Copy link
Contributor Author

antego commented May 7, 2022

It's a good idea to have a PR dedicated to lowering, one to name resolution, one to type checking, one to backend compilation etc.

Thanks! I'll create separate PRs.

Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

I think that overall this looks good :)

gcc/rust/hir/rust-ast-lower-block.h Outdated Show resolved Hide resolved
@@ -120,6 +120,39 @@ class ASTLoweringIfBlock : public ASTLoweringBase
bool terminated;
};

class ASTLoweringIfLetBlock : public ASTLoweringBase
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you chose to add a new visitor class instead of simply implementing the function in ASTLoweringIfBlock? It seems you're not using new class members or anything, so it might be simpler to reuse this existing class.

Copy link
Contributor Author

@antego antego May 9, 2022

Choose a reason for hiding this comment

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

The only reason is because the translated field in the ASTLoweringIfBlock is of type HIR::IfExpr and can't accomodate the HIR::IfLetExpr.

Copy link
Member

Choose a reason for hiding this comment

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

Then I think it would make more sense to turn that into an HIR::ExprWithBlock * but @philberty might think your approach is better. I'm honestly fine with both, I just think it's easier for you to not have to reimplement an entire class/visitor :)

Copy link
Member

Choose a reason for hiding this comment

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

Refactoring to use ExprWithBlock might be a really good idea to cleanup the code a bit here but lets do that as a separate piece since it will affect other code we can raise this as a good-first-pr refactor.

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

LGTM we can do the refactor down the line.

Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented May 10, 2022

Build succeeded:

@bors bors bot merged commit 03c21a0 into Rust-GCC:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants