-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Use interned symbols instead of strings in more places #14855
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
base: master
Are you sure you want to change the base?
Conversation
3f0c7e3
to
408b066
Compare
408b066
to
d961cf4
Compare
Rebased |
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.
Generally looks good, just one question
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
changelog: none