Avoid clone in Comments::next
#125055
Merged
Avoid clone in Comments::next
#125055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current way of stepping through each comment in `Comments` is a bit weird. There is a `Vec<Comments>` and a `current` index, which is fine. The `Comments::next` method clones the current comment but doesn't advance `current`; the advancing instead happens in `print_comment`, which is where each cloned comment is actually finally used (or not, in some cases, if the comment fails to satisfy a predicate). This commit makes things more iterator-like: - `Comments::next` now advances `current` instead of `print_comment`. - `Comments::peek` is added so you can inspect a comment and check a predicate without consuming it. - This requires splitting `PrintState::comments` into immutable and mutable versions. The commit also moves the ref inside the `Option` of the return type, to save callers from having to use `as_ref`/`as_mut`. - It also requires adding `PrintState::peek_comment` alongside the existing `PrintState::next_comment`. (The lifetimes in the signature of `peek_comment` ended up more complex than I expected.) We now have a neat separation between consuming (`next`) and non-consuming (`peek`) uses of each comment. As well as being clearer, this will facilitate the next commit that avoids unnecessary cloning.
This avoids the need for a clone, fixing a FIXME comment.
rustbot
added
S-waiting-on-review
|
cc @fee1-dead |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 13, 2024
…r-errors Avoid clone in `Comments::next` `Comments::next`, in `rustc_ast_pretty`, has this comment: ``` // FIXME: This shouldn't probably clone lmao ``` The obvious thing to try is to return `Option<&Comment>` instead of `Option<Comment>`. But that leads to multiple borrows all over the place, because `Comments` must be borrowed from `PrintState` and then processed by `&mut self` methods within `PrintState`. This PR instead rearranges things so that comments are consumed as they are used, preserving the `Option<Comment>` return type without requiring any cloning. r? `@compiler-errors`
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
...failed to download tidy-html? |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 13, 2024
…r-errors Avoid clone in `Comments::next` `Comments::next`, in `rustc_ast_pretty`, has this comment: ``` // FIXME: This shouldn't probably clone lmao ``` The obvious thing to try is to return `Option<&Comment>` instead of `Option<Comment>`. But that leads to multiple borrows all over the place, because `Comments` must be borrowed from `PrintState` and then processed by `&mut self` methods within `PrintState`. This PR instead rearranges things so that comments are consumed as they are used, preserving the `Option<Comment>` return type without requiring any cloning. r? `@compiler-errors`
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Clearly another CI issue. @bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (982c9c1): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 677.304s -> 677.638s (0.05%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Comments::next, inrustc_ast_pretty, has this comment:The obvious thing to try is to return
Option<&Comment>instead ofOption<Comment>. But that leads to multiple borrows all over the place, becauseCommentsmust be borrowed fromPrintStateand then processed by&mut selfmethods withinPrintState.This PR instead rearranges things so that comments are consumed as they are used, preserving the
Option<Comment>return type without requiring any cloning.r? @compiler-errors