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.
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.
I see that intersperse is used with strs elsewhere, so .intersperse(" or ") should work without the to_string conversion?
There was a problem hiding this comment.
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.
@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.
Having an issue for replacing all joins with intersperses 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