Skip to content

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

Merged
calebcartwright merged 6 commits into
rust-lang:masterfrom
calebcartwright:reorganize-parsing
Dec 21, 2021
Merged

refactor: encapsulate all parsing touchpoints in a single mod#5143
calebcartwright merged 6 commits into
rust-lang:masterfrom
calebcartwright:reorganize-parsing

Conversation

@calebcartwright

Copy link
Copy Markdown
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

ytmimi commented Dec 21, 2021

Copy link
Copy Markdown
Contributor

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!

@ytmimi ytmimi added the pr-ready-to-merge Status: PR is largely ready for merge, waiting for secondary review / last nits label Dec 21, 2021
@calebcartwright

Copy link
Copy Markdown
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

Labels

pr-ready-to-merge Status: PR is largely ready for merge, waiting for secondary review / last nits

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants