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

parse_tt refactorings #94798

Merged
merged 6 commits into from
Mar 12, 2022
Merged

parse_tt refactorings #94798

merged 6 commits into from
Mar 12, 2022

Conversation

nnethercote
Copy link
Contributor

Some readability improvements.

r? @petrochenkov

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

@petrochenkov
Copy link
Contributor

thread 'rustc' panicked at 'assertion failed: matches!(item.top_elts, Tt(TokenTree :: Sequence(..)))', compiler/rustc_expand/src/mbe/macro_parser.rs:520:13

@rustbot author

@rustbot 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 Mar 10, 2022
lqd
lqd reviewed Mar 10, 2022
View reviewed changes
compiler/rustc_expand/src/mbe/macro_parser.rs
Comment on lines +808 to +812
.collect::<Vec<String>>()
.join(" or ");
Copy link
Member

@lqd lqd Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not a big deal in the diagnostics code path of course, but the iter_intersperse feature could help avoid the intermediate Vec<String> (and still not as good as being able to intersperse an &str like join allows) with something similar to:

Suggested change
.collect::<Vec<String>>()
.join(" or ");
.intersperse(" or ".to_string())
.collect::<String>();

(now that I think about it, I'm not sure if intersperse clones the separator each time in this situation, and this could be a bad suggestion...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that intersperse is used with strs elsewhere, so .intersperse(" or ") should work without the to_string conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was pre-existing, I just moved it. It's an error path, and the suggested code isn't shorter or significantly more readable. I don't think a change is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

@petrochenkov IIRC intersperse requires the same type as the items, here a list of strings, but this probably could be refactored to work indeed.

It doesn't matter much of course in this unlikely code path. It could be nice if someone wanted to do that later (and e.g. turn many of the comments in this file into actual doc comments at the same time). Maybe a tiny fixme would be good ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having an issue for replacing all joins with intersperses where possible would probably be more useful than a FIXME for this specific case.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@nnethercote
Refactor the second half of parse_tt.
09c3e82 
The current structure makes it hard to tell that there are just four
distinct code paths, depending on how many items there are in `bb_items`
and `next_items`. This commit introduces a `match` that clarifies
things.
@nnethercote
Disallow TokenTree::{MetaVar,MetaVarExpr} in matchers.
4d4baf7 
They should only appear in transcribers.
@nnethercote
Add a useful assertion.
9f0798b
@nnethercote
Move eof_items handling entirely within inner_parse_loop.
c13ca42 
Also rename `inner_parse_loop` as `parse_tt_inner`, because it's no
longer just a loop.
@nnethercote
Make next_items a SmallVec.
235a87f 
For consistency, and to make the code slightly nicer.
@nnethercote
Move a parse_tt error case into a separate function.
95d13fa
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 11, 2022 •
edited
Loading

I have rebased for the conflicts caused by #94824. The second commit ("Disallow TokenTree::{MetaVar,MetaVarExpr} in matchers") was affected.

@c410-f3r: this may be of interest to you.

@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 Mar 11, 2022
@petrochenkov
Copy link
Contributor

r=me with #94798 (comment) addressed.

@petrochenkov petrochenkov 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 Mar 11, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 11, 2022

📌 Commit 95d13fa has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 11, 2022
@Dylan-DPC
Rollup merge of rust-lang#94798 - nnethercote:parse_tt-refactor, r=pe…
3b30d7d 
…trochenkov

`parse_tt` refactorings

Some readability improvements.

r? `@petrochenkov`
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 11, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2022
@bors
Auto merge of rust-lang#94860 - Dylan-DPC:rollup-n06j8h6, r=Dylan-DPC
2c6a29a 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#87618 (Add missing documentation for std::char types)
 - rust-lang#94769 (Collapse blanket and auto-trait impls by default)
 - rust-lang#94798 (`parse_tt` refactorings)
 - rust-lang#94818 (Rename `IntoFuture::Future` to `IntoFuture::IntoFuture`)
 - rust-lang#94827 (CTFE/Miri: detect out-of-bounds pointers in offset_from)
 - rust-lang#94838 (Make float parsing docs more comprehensive)
 - rust-lang#94839 (Suggest using double colon when a struct field type include single colon)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 86376e3 into rust-lang:master Mar 12, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 12, 2022
@nnethercote nnethercote deleted the parse_tt-refactor branch March 12, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@lqd lqd lqd left review comments

Assignees

@petrochenkov petrochenkov

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

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @petrochenkov @rust-log-analyzer @bors @lqd @rust-highfive @rustbot