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

syntax: Unify macro and attribute arguments in AST #66935

Merged
merged 6 commits into from
Dec 3, 2019
Merged

syntax: Unify macro and attribute arguments in AST #66935

merged 6 commits into from
Dec 3, 2019

Conversation

petrochenkov
Copy link
Contributor

The unified form (ast::MacArgs) represents parsed arguments instead of an unstructured token stream that was previously used for attributes.
It also tracks some spans and delimiter kinds better for fn-like macros and macro definitions.

I've been talking about implementing this with @nnethercote in #65750 (comment).
The parsed representation is closer to MetaItem and requires less token juggling during conversions, so it potentially may be faster.

r? @Centril

@petrochenkov
Copy link
Contributor Author

TODO (not in this PR):

  • Eliminate the use of outer_tokens from parse_in_attr (i.e. avoid reconstructing the token stream with delimiters just to re-parse it again immediately). @Centril, you touched this code last, perhaps you can do this? Otherwise I'll try to implement it next weekend.
  • MacArgs is large, for fn-like macros I had to keep it in a Box to keep static_assert_size!(Expr, 96) happy. Perhaps it needs to be boxed for attributes as well, need to measure.

@petrochenkov
Copy link
Contributor Author

Oh, this PR also basically implements "macro constants" in terms of representation (except they are still an error during parsing).

@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 1, 2019

⌛ Trying commit eadf48aec3e87d05c7101a92612ad094c4e0ea92 with merge 6858a2a7cb74e10251ba66cd547b555717bfaff2...

Centril
Centril reviewed Dec 1, 2019
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Looks good; some minor nits is all I have.

src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/mod.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/item.rs Show resolved Hide resolved
src/librustc_parse/lib.rs Show resolved Hide resolved
@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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 Dec 1, 2019
@bors
Copy link
Contributor

bors commented Dec 1, 2019

☀️ Try build successful - checks-azure
Build commit: 6858a2a7cb74e10251ba66cd547b555717bfaff2 (6858a2a7cb74e10251ba66cd547b555717bfaff2)

@rust-timer
Copy link
Collaborator

Queued 6858a2a7cb74e10251ba66cd547b555717bfaff2 with parent 4007d4e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 6858a2a7cb74e10251ba66cd547b555717bfaff2, comparison URL.

@nnethercote
Copy link
Contributor

A very slight performance improvement.

@petrochenkov
Copy link
Contributor Author

Updated.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2019
@Centril
Copy link
Contributor

Centril commented Dec 3, 2019

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2019

📌 Commit 498737c has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge of rust-lang#66935 - petrochenkov:attrtok2, r=Centril
a7e51b5 
syntax: Unify macro and attribute arguments in AST

The unified form (`ast::MacArgs`) represents parsed arguments instead of an unstructured token stream that was previously used for attributes.
It also tracks some spans and delimiter kinds better for fn-like macros and macro definitions.

I've been talking about implementing this with @nnethercote in rust-lang#65750 (comment).
The parsed representation is closer to `MetaItem` and requires less token juggling during conversions, so it potentially may be faster.

r? @Centril
@Centril Centril mentioned this pull request Dec 3, 2019
Closed
@petrochenkov petrochenkov mentioned this pull request Dec 3, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge of rust-lang#66935 - petrochenkov:attrtok2, r=Centril
cf937fa 
syntax: Unify macro and attribute arguments in AST

The unified form (`ast::MacArgs`) represents parsed arguments instead of an unstructured token stream that was previously used for attributes.
It also tracks some spans and delimiter kinds better for fn-like macros and macro definitions.

I've been talking about implementing this with @nnethercote in rust-lang#65750 (comment).
The parsed representation is closer to `MetaItem` and requires less token juggling during conversions, so it potentially may be faster.

r? @Centril
@Centril Centril mentioned this pull request Dec 3, 2019
Merged
bors added a commit that referenced this pull request Dec 3, 2019
@bors
Auto merge of #66982 - Centril:rollup-yq2281i, r=Centril
f577b0e 
Rollup of 6 pull requests

Successful merges:

 - #66148 (Show the sign for signed ops on `exact_div`)
 - #66651 (Add `enclosing scope` parameter to `rustc_on_unimplemented`)
 - #66904 (Adding docs for keyword match, move)
 - #66935 (syntax: Unify macro and attribute arguments in AST)
 - #66941 (Remove `ord` lang item)
 - #66967 (Remove hack for top-level or-patterns in match checking)

Failed merges:

r? @ghost
@bors bors merged commit 498737c into rust-lang:master Dec 3, 2019
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Dec 3, 2019
@flip1995
Rustup to rust-lang/rust#66935
3a1b0d3
@flip1995 flip1995 mentioned this pull request Dec 3, 2019
Merged
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Dec 3, 2019
@flip1995
Rustup to rust-lang/rust#66935
f6a75f1
bors added a commit to rust-lang/rust-clippy that referenced this pull request Dec 3, 2019
@bors
Auto merge of #4878 - flip1995:rustup, r=flip1995
f0fc18a 
Rustup

Included rustups:

- rust-lang/rust#66935 (syntax: Unify macro and attribute arguments in AST)
- rust-lang/rust#66941 (Remove `ord` lang item)

Fixes? #2597

changelog: none
@petrochenkov petrochenkov mentioned this pull request Dec 3, 2019
Closed
@petrochenkov petrochenkov mentioned this pull request Apr 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

Assignees

@Centril Centril

Labels
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.
5 participants
@petrochenkov @rust-timer @bors @nnethercote @Centril