Clippy: Fixed some clippy warnings #31818
Merged
Conversation
This file contains 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
mukilan
reviewed
Mar 22, 2024
components/script/dom/htmlelement.rs
Outdated
| } | ||
|
|
||
| fn is_ascii_lowercase(c: char) -> bool { | ||
| 'a' <= c && c <= 'w' | ||
| ('a'..='w').contains(&c) |
There was a problem hiding this comment.
It seems weird that we use a to w as the range instead of a to z. Perhaps it was a bug introduced in this commit? It seems that commit was trying to remove the use of the standard library methods char::is_ascii_{lower,upper}case because they were marked unstable at that time. But looks like those methods have been stabilized since then, so perhaps we can simply remove our definitions in this file and call the std methods as before?
There was a problem hiding this comment.
That is almost certainly an Azerty keyboard layout mistake.
There was a problem hiding this comment.
I have made the recommended changes.
components/script/dom/htmlelement.rs
Outdated
| @@ -623,7 +623,7 @@ impl HTMLElement { | |||
| .chars() | |||
| .skip_while(|&ch| ch != '\u{2d}') | |||
| .nth(1) | |||
| .map_or(false, |ch| ch >= 'a' && ch <= 'z') | |||
| .map_or(false, |ch| ('a'..='z').contains(&ch)) | |||
There was a problem hiding this comment.
I think we can use std::char::is_ascii_lowercase here as well.
.map_or(false, |ch| ch.is_ascii_lowercase())
mukilan
reviewed
Mar 22, 2024
components/script/dom/htmlelement.rs
Outdated
| } | ||
|
|
||
| fn is_ascii_lowercase(c: char) -> bool { | ||
| 'a' <= c && c <= 'w' | ||
| c.is_lowercase() |
There was a problem hiding this comment.
is_lowercase has different semantics than is_ascii_lowercase. The former will return true even for lowercase non-ASCII characters such as 'δ' whereas the latter will only check for lowercase characters in the ASCII range (i.e a to z).
Since we want to preserve the semantics, in my previous comment what I was suggesting was to delete the fn is_ascii_lowercase defined in this file and instead use is_ascii_lowercase provided by the std library.
For example, in this line:
if is_ascii_lowercase(next_char) {
result.push(next_char.to_ascii_uppercase());
}
we are calling our own definition of is_ascii_lowercase. We can instead replace that with a call to the std library version like so:
if next_char.is_ascii_lowercase() {
result.push(next_char.to_ascii_uppercase());
}
Similarly, the definition of is_ascii_uppercase can be deleted and the calls in this file can be replaced with the calls to the std library version.
There was a problem hiding this comment.
Ohh, I wasn't aware of the difference between them. I will change them.
mukilan
requested changes
Mar 22, 2024
components/script/dom/htmlelement.rs
Outdated
| @@ -560,11 +560,11 @@ static DATA_PREFIX: &str = "data-"; | |||
| static DATA_HYPHEN_SEPARATOR: char = '\x2d'; | |||
|
|
|||
| fn is_ascii_uppercase(c: char) -> bool { | |||
| 'A' <= c && c <= 'Z' | |||
| c.is_ascii_uppercase() | |||
| } | |||
|
|
|||
| fn is_ascii_lowercase(c: char) -> bool { | |||
There was a problem hiding this comment.
This function is now redundant, as it is not doing any actual work itself and simply forwards the call to the standard library method. So, we can just delete this function definition from this file and simply call .is_ascii_lowercase() wherever the deleted function is used. Please refer to my previous comment for an example of what needs to be done at the call site.
Similar refactoring needs to done for is_ascii_uppercase as well.
mukilan
approved these changes
Mar 23, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
mukilan
reviewed
Mar 23, 2024
| ParseMode::RelativePlus => value = 3 + value, | ||
| ParseMode::RelativeMinus => value = 3 - value, | ||
| ParseMode::RelativePlus => value += 3, | ||
| ParseMode::RelativeMinus => value -= 3, |
There was a problem hiding this comment.
Since subtraction is not commutative, this change is not equivalent to the original code i.e value -= 3 expands to value = value - 3 whereas we want value = 3 - value
I think we can simply revert the change on line 178 alone.
There was a problem hiding this comment.
I have reverted the changes, it should be fine now.
mukilan
added
T-linux-wpt-2013
github-actions
bot
removed
T-linux-wpt-2013
|
🔨 Triggering try run (#8401592794) for Linux WPT |
|
Test results for linux-wpt-layout-2013 from try job (#8401592794): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (14)
|
|
Test results for linux-wpt-layout-2020 from try job (#8401592794): Flaky unexpected result (18)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#8401592794) succeeded. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixed some clippy warnings.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors