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

refactor: encapsulate all parsing touchpoints in a single mod #5143

Merged
merged 6 commits into from
Dec 21, 2021

Conversation

calebcartwright
Copy link
Member

r? @ytmimi - best reviewed commit by commit

This is the type of thing I was referring to the other day. The blast radius of rustc_parse touch points was supposed to be contained within our syntux module (now renamed to parse), but particularly around macro handling that encapsulation had been broken/never fully encapsulated.

This just pulls everything into one place almost exclusively via copy/paste and renames (delta between lines added and removed is just due to the greater number of smaller files, resulting in some extra lines for imports).

However, the asm! changes aren't pulled in yet because i forget to update the visibility on the asm mod in the compiler 😢

@ytmimi
Copy link
Contributor

ytmimi commented Dec 21, 2021

Thanks for asking me to do the review.

I went through all the commits like you suggested and I think the way you chose to refactor / encapsulate the parsing logic made a lot of sense.

I like how cfg_if and lazy_static mods are really focused, and I think it gives us a good template for parsing the asm macro when all the required changes have landed in the compiler! I also think that the parse_cfg_if and parse_lazy_static are nice abstractions around parsing each of those macros.

I'm happy to move forward with these changes!

@calebcartwright
Copy link
Member Author

Thanks!

@calebcartwright calebcartwright merged commit 7b8303d into rust-lang:master Dec 21, 2021
@calebcartwright calebcartwright deleted the reorganize-parsing branch December 21, 2021 16:55
@ytmimi ytmimi mentioned this pull request Jan 25, 2024
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.

2 participants