Use interned symbols instead of strings in more places #14855
Conversation
rustbot
added
the
S-waiting-on-review
samueltardieu
force-pushed
the
more-symbols
branch
2 times, most recently
from
408b066 to
d961cf4
Compare
|
Rebased |
| complete_path: String, | ||
| parent: &[Symbol], | ||
| complete_path: impl FnOnce(&str) -> String, | ||
| ) { | ||
| match attr_paths.entry(complete_path) { | ||
| let parent = parent.iter().join(":"); | ||
| match attr_paths.entry(complete_path(&parent)) { |
There was a problem hiding this comment.
Is there a reason for this closure-based change? The join + closure call (which invokes format!()) looks like it's doing essentially the same thing as before which had the format!() directly inlined as the function argument, or am I missing a subtle difference? (The change from Vec<String> to Vec<Symbol> is nice though)
There was a problem hiding this comment.
Ah, reading the other comment from Alexendoo, I guess the variable declaration line above used to be way longer and you wanted to share that between the two calls? It's not really much longer anymore now and the closure makes it slightly harder to tell what's going on but that's a bit pedantic, if you still think that's an improvement then I'm fine merging it as is too
There was a problem hiding this comment.
You're right, it is best to leave it quasi-untouched, as to focus on the meaning of this PR only. Only a .iter() needs to be added, I've pushed the change.
samueltardieu
force-pushed
the
more-symbols
branch
from
d961cf4 to
3ed210c
Compare
changelog: none