Skip to content
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

Fix clippy warnings for lib, examples, and tests #1585

Closed
wants to merge 3 commits into from

Conversation

cky
Copy link
Contributor

@cky cky commented Dec 10, 2022

Similar in concept to #1569, but:

  • Fixes many more Clippy warnings (lib, examples, and tests; note that I fix the main project only, not benchmarks)
  • Implements the fixes in idiomatic ways, not just "how Clippy suggests" (which often results in unidiomatic code)
  • Does not bump the MSRV (since that can't be done without bump the major version)

I'll also add PR comments in places where the code changes are not immediately obvious.

@cky cky force-pushed the fix-clippy-warnings branch from f1690e9 to 3ee23fe Compare December 10, 2022 08:19
@@ -109,7 +109,7 @@ impl AsBytes for str {
impl<'a> AsBytes for &'a [u8] {
#[inline(always)]
fn as_bytes(&self) -> &[u8] {
*self
self
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes clippy::explicit_auto_deref.

@@ -148,7 +148,8 @@ as_bytes_array_impls! {
}

/// Transforms common types to a char for basic token parsing
pub trait AsChar {
#[allow(clippy::len_without_is_empty)]
pub trait AsChar: Copy {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Copy constraint addresses clippy::wrong_self_convention.

@@ -49,7 +49,7 @@ pub fn is_digit(chr: u8) -> bool {
/// ```
#[inline]
pub fn is_hex_digit(chr: u8) -> bool {
(chr >= 0x30 && chr <= 0x39) || (chr >= 0x41 && chr <= 0x46) || (chr >= 0x61 && chr <= 0x66)
matches!(chr, 0x30..=0x39 | 0x41..=0x46 | 0x61..=0x66)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Clippy's suggested fixes for clippy::manual_range_contains are unidiomatic for this use case (as well as other matches! fixes in this branch):

(0x30..=0x39).contains(&chr) || (0x41..=0x46).contains(&chr) || (0x61..=0x66).contains(&chr)

src/number/complete.rs Show resolved Hide resolved
Comment on lines -2004 to +2011
let larger = format!("{}", test);
assert_parse!(recognize_float(&larger[..]), Ok(("", test)));
assert_parse!(recognize_float(test), Ok(("", test)));

assert_parse!(float(larger.as_bytes()), Ok((&b""[..], expected32)));
assert_parse!(float(&larger[..]), Ok(("", expected32)));
assert_parse!(float(test.as_bytes()), Ok((&b""[..], expected32)));
assert_parse!(float(test), Ok(("", expected32)));

assert_parse!(double(larger.as_bytes()), Ok((&b""[..], expected64)));
assert_parse!(double(&larger[..]), Ok(("", expected64)));
assert_parse!(double(test.as_bytes()), Ok((&b""[..], expected64)));
assert_parse!(double(test), Ok(("", expected64)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses clippy::useless_format.

While it's easy to simply replace the format!("{}", test) with test.to_string() (as suggested by Clippy) or even test.to_owned(), neither of these address the fact that there's actually no reason to create a String here in the first place.

The reason a String is created for the streaming test is because the test needed to append a garbage character to avoid an Err::Incomplete result. There's no such need for the complete test.

Comment on lines -305 to -313
c == 'β'
|| c == 'è'
|| c == 'ƒ'
|| c == 'ô'
|| c == 'ř'
|| c == 'è'
|| c == 'Â'
|| c == 'ß'
|| c == 'Ç'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change addresses clippy::nonminimal_bool. The expression is non-minimal because "è" is compared twice. While it's easy enough to just remove the duplicate, it seems a missed opportunity to optimise the comparison using matches!.

@@ -1381,7 +1378,7 @@ impl HexDisplay for [u8] {
v.push(b'\t');

for &byte in chunk {
if (byte >= 32 && byte <= 126) || byte >= 128 {
if matches!(byte, 32..=126 | 128..=255) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current versions of Rust supports using 128.. in patterns. But Rust 1.48 doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it fails on matches too: https://github.com/rust-bakery/nom/actions/runs/3807696627/jobs/6477623360#step:5:29
I'm releasing nom 7.2 soon, then for 8.0 I'll raise the minimum version.

@Geal Geal self-requested a review as a code owner December 30, 2022 15:33
@coveralls
Copy link

coveralls commented Dec 30, 2022

Pull Request Test Coverage Report for Build 3818917772

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 9 of 15 (60.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.07%) to 62.086%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/internal.rs 0 1 0.0%
src/traits.rs 5 10 50.0%
Files with Coverage Reduction New Missed Lines %
src/internal.rs 1 22.35%
src/number/complete.rs 1 60.0%
Totals Coverage Status
Change from base Build 3817958594: 0.07%
Covered Lines: 1518
Relevant Lines: 2445

💛 - Coveralls

@Geal
Copy link
Collaborator

Geal commented Jan 1, 2023

merged in 12546b0 (had to edit it to set the minimal version in CI to 1.42), thanks!

@Geal Geal closed this Jan 1, 2023
@Geal Geal added this to the 8.0 milestone Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants