Refactor is_case_difference function
#115258
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
WaffleLapkin
left a comment
WaffleLapkin
left a comment
There was a problem hiding this comment.
I don't think this function needs optimization, but your code is generally nicer and more documented here, so let's go with that.
I've left a few nitpicks, don't mind them much, it's always easy to find things to nitpick about on small PRs like there 😅
Thank you for the pull request!
| pub fn is_case_difference(sm: &SourceMap, suggested: &str, sp: Span) -> bool { | ||
| // FIXME: this should probably be extended to also account for `FO0` → `FOO` and unicode. | ||
| // Retrieve code | ||
| let found = match sm.span_to_snippet(sp) { |
Member
There was a problem hiding this comment.
could you turn this into a let-else?
| && found != suggested | ||
|
|
||
| // ASCII confusable characters when capitalization. | ||
| const ASCII_CONFUSABLES: [char; 12] = ['c', 'f', 'i', 'k', 'o', 's', 'u', 'v', 'w', 'x', 'y', 'z']; |
Member
There was a problem hiding this comment.
Using a const does not make a difference, actually. Consts are copied on access, so this is actually worse, if anything (as we now potentially spill two copies on the attach, instead of one).
Not that it actually matters, given than this is the error path, but still. You should probably change the type to &[char; 12] if you want to make it const (do that only the reference is copied, not the array).
| // ASCII confusable characters when capitalization. | ||
| const ASCII_CONFUSABLES: [char; 12] = ['c', 'f', 'i', 'k', 'o', 's', 'u', 'v', 'w', 'x', 'y', 'z']; | ||
|
|
||
| // # CHECK 1 |
Member
There was a problem hiding this comment.
Hm, do you think those titles are useful? To me they seem somewhat pointless, they don't really add information or make it easier to scan the code, I think?
Contributor
Author
There was a problem hiding this comment.
In the original code, the comment sounds like // All the chars that differ in capitalization are confusable (above):
Should I make it more descriptive?
Member
There was a problem hiding this comment.
I'm not sure what you mean, I was only talking about the # CHECK titles, the comments themself look fine.
Comment on lines
+2737
to
+2739
| // 'zip' function is used to iterate over two sequences in parallel | ||
| found.chars() | ||
| .zip(suggested.chars()) |
Member
There was a problem hiding this comment.
There is no need to explain zip, it's a well known function :)
Additionally I'd suggest using iter::zip, in my opinion it's nicer, as it highlights the symmetry.
Suggested change
| // 'zip' function is used to iterate over two sequences in parallel | |
| found.chars() | |
| .zip(suggested.chars()) | |
| iter::zip(found.chars(), suggested.chars()) |
Contributor
Author
There was a problem hiding this comment.
Thank you for the remainder :)
Actually, I'm still new to Rust, and there's a lot I'm still catching on to.
About the iter::zip, I completely agree with you. I'll modify it later.
| // - Check character `f` is present in the `ASCII_CONFUSABLES` array, OR | ||
| // - Check character `s` is present in the `ASCII_CONFUSABLES` array | ||
| // | ||
| // This line equivalent to the: .filter(|(f, s)| f != s).all(|(f, s)| (ascii_confusables.contains(&f) || ascii_confusables.contains(&s))); |
Member
There was a problem hiding this comment.
I think this is redundant, the filter representation is more confusing if anything
Member
|
@rustbot author |
rustbot
added
S-waiting-on-author
Contributor
Author
|
@rustbot review |
rustbot
added
S-waiting-on-review
Member
|
@allaboutevemirolive You have not materially responded to the reviewer's comments. Please do not return this to the reviewer's queue without making some meaningful change or at least requesting distinctive feedback. @rustbot author |
rustbot
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Previous code was refactored to remove unnecessary memory allocation and to include comments for clarity.
Some of the current code's optimizations include:
eq_ignore_ascii_case()instead ofto_lowercase()to avoid memory allocations and performance overhead.all()and logical conditions, reducing the overall number of operations.ascii_confusablesby usingconst.constvalues are evaluated and resolved at compile time, which means they don't result in runtime allocations.Testing
This improvement was tested with command
./x test --stage 2 tests/uion Debian.