Skip to content

syntax: Unify macro and attribute arguments in AST - #66935

Merged
bors merged 6 commits into
rust-lang:masterfrom
petrochenkov:attrtok2
Dec 3, 2019
Merged

syntax: Unify macro and attribute arguments in AST#66935
bors merged 6 commits into
rust-lang:masterfrom
petrochenkov:attrtok2

Conversation

@petrochenkov

@petrochenkovpetrochenkov commented Dec 1, 2019

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

petrochenkov commented Dec 1, 2019

Copy link
Copy Markdown
ContributorAuthor

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

petrochenkov commented Dec 1, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@petrochenkov

petrochenkov commented Dec 1, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors try @rust-timer queue

@rust-timer

rust-timer commented Dec 1, 2019

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Dec 1, 2019

Copy link
Copy Markdown
Collaborator

⌛ Trying commit eadf48aec3e87d05c7101a92612ad094c4e0ea92 with merge 6858a2a7cb74e10251ba66cd547b555717bfaff2...

Centril
Centril reviewed Dec 1, 2019
View reviewed changes

@CentrilCentril left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/librustc_parse/parser/mod.rs Outdated
Comment threadsrc/librustc_parse/parser/item.rs Outdated
Comment threadsrc/librustc_parse/lib.rs Outdated
@CentrilCentril 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

bors commented Dec 1, 2019

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Dec 1, 2019

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Dec 2, 2019

Copy link
Copy Markdown
Collaborator

Finished benchmarking try commit 6858a2a7cb74e10251ba66cd547b555717bfaff2, comparison URL.

@nnethercote

nnethercote commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

A very slight performance improvement.

@petrochenkov

petrochenkov commented Dec 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated.

@petrochenkovpetrochenkov 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

Centril commented Dec 3, 2019

Copy link
Copy Markdown
Contributor

Thanks! @bors r+

@bors

bors commented Dec 3, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 498737c has been approved by Centril

@borsbors 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 ofrust-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
@CentrilCentril mentioned this pull request Dec 3, 2019
Closed
@petrochenkovpetrochenkov 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 ofrust-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
@CentrilCentril 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:masterDec 3, 2019
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Dec 3, 2019
@flip1995
Rustup torust-lang/rust#66935
3a1b0d3
@flip1995flip1995 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 torust-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
@petrochenkovpetrochenkov mentioned this pull request Dec 3, 2019
Closed
@petrochenkovpetrochenkov mentioned this pull request Apr 19, 2022
Closed
@petrochenkov
petrochenkov deleted the attrtok2 branch February 22, 2025 18:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@CentrilCentrilCentril left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@CentrilCentril

Labels

S-waiting-on-borsStatus: 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