Skip to content

rustc_ast_pretty cleanups #117928

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
Merged
merged 11 commits into from
Nov 22, 2023
Merged

rustc_ast_pretty cleanups #117928

merged 11 commits into from
Nov 22, 2023

Conversation

nnethercote
Copy link
Contributor

Some improvements I found while looking at this code.

r? @fee1-dead

@rustbot 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 Nov 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 15, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@bors
Copy link
Collaborator

bors commented Nov 17, 2023

☔ The latest upstream changes (presumably #117979) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Remove unneeded features.
2f51f0b
@nnethercote
Remove unnecessary derives.
1d320ed
@nnethercote
Update itertools to 0.11.
3eadc68 
Because the API for `with_position` improved in 0.11 and I want to use
it.
@nnethercote
Remove IterDelimited.
ce363bb 
itertools has `with_position` which does the same thing.
@nnethercote
Remove NO_ANN.
33cee0b 
This makes `rustc_hir_pretty` more like `rustc_ast_pretty`.
@nnethercote
Remove or downgrade unnecessary pub visibility markers.
d9443f7
@nnethercote
Remove unused PrintState::generic_params_to_string method.
c6a9027
@nnethercote
Streamline PrintState.
e16b52d 
`PrintState` is a trait containing code that can be used by both AST and
HIR pretty-printing. But several of its methods are only used by AST
printing.

This commit moves those methods out of the trait and into the AST
`State` impl, so they are not exposed unnecessarily. This commit also
removes four unused methods: `param_to_string`,
`foreign_item_to_string`, `assoc_item_to_string`, and
`print_inner_attributes_inline`.
@nnethercote
Factor out common code in PrintState.
10c8b56 
The AST and HIR versions of `State::print_ident` are textually
identical, but the types differ slightly. This commit factors out the
common code they both have by replacing `print_ident` with `ann_post`,
which is a smaller function that still captures the type difference.
fee1-dead
fee1-dead approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Sorry for the late review! Overall LGTM. r=me after one small nit.

@nnethercote
Copy link
Contributor Author

nnethercote commented Nov 22, 2023 •
edited
Loading

I addressed the nit.

@bors r=fee1-dead

@bors
Copy link
Collaborator

bors commented Nov 22, 2023

📌 Commit 6686221 has been approved by fee1-dead

It is now in the queue for this repository.

@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 Nov 22, 2023
@bors
Copy link
Collaborator

bors commented Nov 22, 2023

⌛ Testing commit 6686221 with merge cc4bb0d...

@bors
Copy link
Collaborator

bors commented Nov 22, 2023

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing cc4bb0d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 22, 2023
@bors bors merged commit cc4bb0d into rust-lang:master Nov 22, 2023
@rustbot rustbot added this to the 1.76.0 milestone Nov 22, 2023
@bors bors mentioned this pull request Nov 22, 2023
Merged
@nnethercote nnethercote deleted the rustc_ast_pretty branch November 22, 2023 08:07
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cc4bb0d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)		 0.7% [0.7%, 0.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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: 676.959s -> 676.862s (-0.01%)
Artifact size: 313.78 MiB -> 313.76 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead approved these changes

Assignees

@fee1-dead fee1-dead

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. 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.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @rustbot @bors @rust-timer @fee1-dead