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.

Sign up for GitHub

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

Jump to bottom

Cleanup rmeta::MacroDef #66364

Merged
merged 7 commits into from
Mar 10, 2020
Merged

Cleanup rmeta::MacroDef #66364

merged 7 commits into from
Mar 10, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 13, 2019 •
edited by eddyb
Loading

Avoid using rountrip parsing in the encoder and in fn load_macro_untracked.

The main reason I was interested in this was to remove rustc_parse as a dependency of rustc_metadata but it seems like this had other benefits as well.

Fixes #49511.

r? @eddyb
cc @matthewjasper @estebank @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2019
Mark-Simulacrum
Mark-Simulacrum reviewed Nov 13, 2019
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Overall though I think this is reasonable, I guess :)

src/librustc_metadata/rmeta/decoder.rs Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov self-assigned this Nov 13, 2019
@bors

This comment has been minimized.

Sign in to view
@Centril
Copy link
Contributor Author

Centril commented Nov 14, 2019

All the tests passed (before I adjusted the comment), so this should be ready for review now. :)

@rust-highfive

This comment has been minimized.

Sign in to view
@eddyb

This comment has been minimized.

Sign in to view
@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2019
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2019
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2019
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2019
@petrochenkov petrochenkov removed their assignment Nov 17, 2019
@petrochenkov
Copy link
Contributor

Do not forget to run perf on this.
Stringification was a cheat, but it could be a performance-friendly cheat.

@Centril
Copy link
Contributor Author

Centril commented Nov 21, 2019

Let's try a perf run now (and maybe one later after doing some more changes according to #66364 (comment)):

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 21, 2019

⌛ Trying commit a2ec40b4bf40b57c3df20d4c679804812d3be8e6 with merge 8391cac23555ae55fb9f4d7fc909c58c3f1313bf...

@pietroalbini
Copy link
Member

@bors retry

Yield to the beta release

@bors
Copy link
Contributor

bors commented Mar 10, 2020

⌛ Testing commit bafa5cc with merge 1581278...

@bors
Copy link
Contributor

bors commented Mar 10, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov,eddyb
Pushing 1581278 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 10, 2020
@bors bors merged commit 1581278 into rust-lang:master Mar 10, 2020
This was referenced Mar 10, 2020
Closed
Merged
Merged
Merged
@Centril Centril deleted the cleanup-macro-def branch March 10, 2020 20:55
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 12, 2020
@Mark-Simulacrum
Copy link
Member

Tagging as relnotes (so we don't forget about it -- it may not go into the final version), though that may be a false positive. @tomaka reported some breakage here (https://twitter.com/tomaka17/status/1238204339505770499) though it may be entirely in unstable features and unobservable on stable, in which case we wouldn't need to document it in relnotes.

@eddyb tells me @petrochenkov is the person to ask on whether there's something to say in relnotes here :)

@Mark-Simulacrum
Copy link
Member

Further discussion on Discord informed me that perhaps there is not a significant change here and the only exposed changes are to unstable APIs in proc_macro anyway, so untagging.

@Mark-Simulacrum Mark-Simulacrum removed the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 12, 2020
@petrochenkov
Copy link
Contributor

Spans can affect many things, starting from lints (as this PR itself shows).
I guess the beta crater run will show the full extent of changes this PR caused.

@petrochenkov
Copy link
Contributor

Actually, running a separate crater check for this PR would be reasonable.

This was referenced Mar 13, 2020
Merged
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#69988 - petrochenkov:nomacrodef, r=Centril
dcd03aa 
rustc_metadata: Remove `rmeta::MacroDef`

And other related cleanups.

Follow-up to rust-lang#66364.
r? @Centril
Centril added a commit to Centril/rust that referenced this pull request Mar 15, 2020
@Centril
Rollup merge of rust-lang#69988 - petrochenkov:nomacrodef, r=Centril
8bca839 
rustc_metadata: Remove `rmeta::MacroDef`

And other related cleanups.

Follow-up to rust-lang#66364.
r? @Centril
@staktrace staktrace mentioned this pull request Mar 15, 2020
Closed
@ehuss ehuss mentioned this pull request Mar 18, 2020
Closed
This was referenced Mar 25, 2020
Closed
Closed
@ollie27 ollie27 mentioned this pull request Apr 5, 2020
Open
@ehuss ehuss mentioned this pull request Apr 10, 2020
Merged
Centril added a commit to Centril/rust that referenced this pull request Apr 10, 2020
@Centril
Rollup merge of rust-lang#70969 - ehuss:json-filename-macros, r=eddyb
6f118f2 
Fix JSON file_name documentation for macros.

JSON `file_name` paths were changed in rust-lang#66364 for macros to point to actual source files instead of using `<MACRONAME macros>`.

Closes rust-lang#70396
@robjtede robjtede mentioned this pull request Apr 17, 2020
Open
@camelid camelid mentioned this pull request Jun 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

@estebank estebank estebank left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@petrochenkov petrochenkov petrochenkov approved these changes

Assignees

@eddyb eddyb

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

macro_rules macros should not be serialized through strings in crate metadata.