-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
parse_tt
refactorings
#94798
Conversation
Best reviewed one commit at a time. |
@rustbot author |
.collect::<Vec<String>>() | ||
.join(" or "); |
There was a problem hiding this comment.
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:
.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...)
There was a problem hiding this comment.
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 str
s elsewhere, so .intersperse(" or ")
should work without the to_string
conversion?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 join
s with intersperse
s where possible would probably be more useful than a FIXME for this specific case.
This comment has been minimized.
This comment has been minimized.
4619f85
to
2bee213
Compare
This comment was marked as resolved.
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.
2bee213
to
95d13fa
Compare
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. |
r=me with #94798 (comment) addressed. |
@bors r+ |
📌 Commit 95d13fa has been approved by |
…trochenkov `parse_tt` refactorings Some readability improvements. r? `@petrochenkov`
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
Some readability improvements.
r? @petrochenkov