Some parser and AST cleanups #114353

nnethercote · 2023-08-02T01:34:26Z

Things I found while looking closely at this code.

It has a single call site.

This code currently uses a `while` loop and gathers token trees into a vector, but it only succeeds in the case where there is a single token tree. We can instead just call `parse_token_tree` once and then look for `Eof` to detect the success case.

It's more of a `Parser`-level concern than a `TokenCursor`-level concern. Also, `num_bump_calls` is a more accurate name, because it's incremented in `Parser::bump`.

Similar to the last commit, it's more of a `Parser`-level concern than a `TokenCursor`-level concern. And the struct size reductions are nice. After this change, `TokenCursor` is as minimal as possible (two fields and two methods) which is nice.

The `None` variant has already been renamed `Invisible`.

rustbot · 2023-08-02T01:34:33Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

compiler/rustc_builtin_macros/src/assert.rs

petrochenkov · 2023-08-02T03:28:42Z

r=me after addressing #114353 (comment) and fixing doc comments.
@rustbot author

nnethercote · 2023-08-02T09:04:28Z

Thank you for the fast review. I addressed the review comments and fixed the doc comments.

@bors r=petrochenkov rollup

bors · 2023-08-02T09:04:30Z

📌 Commit 60231d6 has been approved by petrochenkov

It is now in the queue for this repository.

…=petrochenkov Some parser and AST cleanups Things I found while looking closely at this code. r? `@petrochenkov`

Noratrieb · 2023-08-02T10:39:50Z

@bors r- CI failure

It's the same as `Delimiter`, minus the `Invisible` variant. I'm generally in favour of using types to make impossible states unrepresentable, but this one feels very low-value, and the conversions between the two types are annoying and confusing. Look at the change in `src/tools/rustfmt/src/expr.rs` for an example: the old code converted from `MacDelimiter` to `Delimiter` and back again, for no good reason. This suggests the author was confused about the types.

nnethercote · 2023-08-02T23:07:55Z

I fixed the rustfmt formatting error.

@bors r=petrochenkov

bors · 2023-08-02T23:07:57Z

📌 Commit d75ee2a has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2023-08-03T04:26:45Z

⌛ Testing commit d75ee2a with merge fb31b3c...

bors · 2023-08-03T06:17:32Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing fb31b3c to master...

rust-timer · 2023-08-03T15:55:48Z

Finished benchmarking commit (fb31b3c): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.7%, -0.6%]	5
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[1.5%, 7.0%]	7
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.497s -> 647.579s (-0.45%)

nnethercote added 7 commits August 2, 2023 10:01

parse_all_token_trees cannot fail.

d9f2fdf

Inline and remove parse_all_token_trees.

db7b07a

It has a single call site.

Only call parse_token_tree once.

203cba7

This code currently uses a `while` loop and gathers token trees into a vector, but it only succeeds in the case where there is a single token tree. We can instead just call `parse_token_tree` once and then look for `Eof` to detect the success case.

Move TokenCursor::num_next_calls into Parser and rename it.

54eb6bc

It's more of a `Parser`-level concern than a `TokenCursor`-level concern. Also, `num_bump_calls` is a more accurate name, because it's incremented in `Parser::bump`.

Avoid an unnecessary local variable.

ac64a53

Fix an erroneous comment.

ba294a8

The `None` variant has already been renamed `Invisible`.

rustbot assigned petrochenkov Aug 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 2, 2023

This comment has been minimized.

Sign in to view

petrochenkov reviewed Aug 2, 2023

View reviewed changes

compiler/rustc_builtin_macros/src/assert.rs Outdated Show resolved Hide resolved

rustbot 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 Aug 2, 2023

nnethercote force-pushed the parser-ast-cleanups branch from 69f8ece to 60231d6 Compare August 2, 2023 09:03

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2023

This comment has been minimized.

Sign in to view

Noratrieb added a commit to Noratrieb/rust that referenced this pull request Aug 2, 2023

Rollup merge of rust-lang#114353 - nnethercote:parser-ast-cleanups, r…

afb15c4

…=petrochenkov Some parser and AST cleanups Things I found while looking closely at this code. r? `@petrochenkov`

Noratrieb mentioned this pull request Aug 2, 2023

Rollup of 5 pull requests #114366

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 2, 2023

nnethercote force-pushed the parser-ast-cleanups branch from 60231d6 to d75ee2a Compare August 2, 2023 23:03

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2023

bors merged commit fb31b3c into rust-lang:master Aug 3, 2023

rustbot added this to the 1.73.0 milestone Aug 3, 2023

bors mentioned this pull request Aug 3, 2023

fix(parse): avoid panic when cfg wrapper by bracket under capture-cfg mode #113056

Closed

nnethercote deleted the parser-ast-cleanups branch August 3, 2023 08:53

Uh oh!

Some parser and AST cleanups #114353

Some parser and AST cleanups #114353

Uh oh!

Conversation

nnethercote commented Aug 2, 2023

Uh oh!

rustbot commented Aug 2, 2023

Uh oh!

This comment has been minimized.

Uh oh!

petrochenkov commented Aug 2, 2023

Uh oh!

nnethercote commented Aug 2, 2023

Uh oh!

bors commented Aug 2, 2023

Uh oh!

This comment has been minimized.

Noratrieb commented Aug 2, 2023

Uh oh!

nnethercote commented Aug 2, 2023

Uh oh!

bors commented Aug 2, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

rust-timer commented Aug 3, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants