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.

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

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
Comment on lines +808 to +812
.collect::<Vec<String>>()
.join(" or ");
Copy link
Member

@lqd lqd Mar 10, 2022

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.

@bors

This comment was marked as resolved.

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.
They should only appear in transcribers.
Also rename `inner_parse_loop` as `parse_tt_inner`, because it's no
longer just a loop.
For consistency, and to make the code slightly nicer.
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 11, 2022

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
…trochenkov

`parse_tt` refactorings

Some readability improvements.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2022
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
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
Development

Successfully merging this pull request may close these issues.

7 participants