Fix clippy warnings #376

lukaslueg · 2017-05-31T20:30:44Z

This fixes all clippy warnings, mostly lifetime elisions and useless borrows. Clippy now passes on nightly; on stable/without clippy, no lint warnings are given. All tests still pass.

I've not moved any code around, which is the reason for allowing string_lit_as_bytes in so many places.

Up for review

lukaslueg · 2017-06-01T07:58:20Z

src/re_bytes.rs

@@ -785,6 +785,7 @@ pub struct Captures<'t> {
    named_groups: Arc<HashMap<String, usize>>,
 }

+#[allow(len_without_is_empty)]


It's probably better to

fn is_empty(&self) -> bool { debug_assert!(self.len() > 0); false }

as this will trigger const-folding and warnings should anyone expect captures to be empty

I don't agree with adding a useless method to the public API.

BurntSushi

@lukaslueg Thanks so much for doing this. I realize it's a chore to do this, and I apologize in advance for being so picky about this stuff, but my comments probably show why I haven't really gotten on board the Clippy train.

At a high level, I agree with a lot of your changes. The amount of superfluous & sigils is amusing, and I wonder if I can blame that on a bygone era when those were actually required.

But there are a number of things I flat out disagree with, and I think some of the transformations you made make the code less readable. On a different note, your use of the {debug_}?assert_ne! macros requires a minimum Rust version bump, which I do not want to do at this time. We need to stick with Rust 1.12.

It seems like the way to express disagreement with Clippy is to pepper the code base with cfg'd attributes, and I simply do not want to deal with that. In principle, I'd be fine if it were rare, but this PR contains a lot of them. They have to go. If there is a way to add a config file to globally disable some lints, then I think I would be OK with the loss of precision.

BurntSushi · 2017-06-01T11:48:02Z

examples/shootout-regex-dna-replace.rs

@@ -9,6 +9,8 @@ macro_rules! regex {
    }}
 }

+#[cfg_attr(not(feature = "cargo-clippy"), allow(unknown_lints))]
+#[allow(regex_macro)]


I do not want to litter the examples directory with a bunch of clippy stuff. We need to find a way to remove this (similarly for the rest of the examples).

BurntSushi · 2017-06-01T11:49:59Z

src/compile.rs

@@ -1049,13 +1055,6 @@ impl ByteClassSet {
    }
 }

-fn u32_to_usize(n: u32) -> usize {


Why did you get rid of this function? Please put it back. Without it, you've turned a hard error into a silent error.

Actually, I think this is the only really useful warning by clippy for the whole codebase: The comparison always evaluates to false, making the check superfluous.

Why is it there anyway? if usize is 32bit, n can't be larger. If usize is 64bit, n can't be larger as well.

It's a guard against the case when usize is less than 32 bits. I'm not sure what the state of 16 bit usizes is in rustc, but IIRC people were interested in making it happen. It's certainly possible, so it seems worthwhile to be conservative.

Shall I leave a comment about std::convert::TryFrom ?

BurntSushi · 2017-06-01T11:50:36Z

src/compile.rs

@@ -713,6 +713,12 @@ impl Compiler {
    }
 }

+impl Default for Compiler {


Is this actually used anywhere? If not, I don't see any reason to add it.

BurntSushi · 2017-06-01T11:51:57Z

src/dfa.rs

@@ -271,7 +271,7 @@ struct State{
    data: Box<[u8]>,
 }

-/// InstPtr is a 32 bit pointer into a sequence of opcodes (i.e., it indexes
+/// `InstPt`r is a 32 bit pointer into a sequence of opcodes (i.e., it indexes


Looks like the ending backtick is in the wrong spot.

(I really don't see why every single identifier needs to be put in backticks. These comments aren't public, so they'll never be rendered anyway.)

BurntSushi · 2017-06-01T11:53:44Z

src/dfa.rs

@@ -482,7 +484,7 @@ impl<'a> Fsm<'a> {
            Some(STATE_DEAD) => return Result::NoMatch(at),
            Some(si) => si,
        };
-        debug_assert!(dfa.start != STATE_UNKNOWN);
+        debug_assert_ne!(dfa.start, STATE_UNKNOWN);


debug_assert_ne! was added in Rust 1.13. regex 0.2's minimum support Rust version is 1.12. You'll need to revert all uses of debug_assert_ne! and assert_ne!.

BurntSushi · 2017-06-01T12:01:18Z

src/re_bytes.rs

@@ -785,6 +785,7 @@ pub struct Captures<'t> {
    named_groups: Arc<HashMap<String, usize>>,
 }

+#[allow(len_without_is_empty)]


I don't agree with adding a useless method to the public API.

BurntSushi · 2017-06-01T12:02:23Z

src/re_bytes.rs

@@ -986,7 +987,7 @@ pub trait Replacer {
    /// be beneficial to avoid finding sub-captures.
    ///
    /// In general, this is called once for every call to `replacen`.
-    fn no_expansion<'r>(&'r mut self) -> Option<Cow<'r, [u8]>> {
+    fn no_expansion(&mut self) -> Option<Cow<[u8]>> {


Please put the lifetimes back. I don't mind elision in simple cases, but I'd rather the lifetime parameter be explicit here, especially in the trait definition.

BurntSushi · 2017-06-01T12:03:00Z

src/re_trait.rs

        self,
-        text: &'t Self::Text,
-    ) -> CaptureMatches<'t, Self> {
+        text: & Self::Text,


There's an extra space here.

BurntSushi · 2017-06-01T12:03:33Z

src/re_unicode.rs

@@ -1198,7 +1199,7 @@ pub trait Replacer {
    /// be beneficial to avoid finding sub-captures.
    ///
    /// In general, this is called once for every call to `replacen`.
-    fn no_expansion<'r>(&'r mut self) -> Option<Cow<'r, str>> {
+    fn no_expansion(&mut self) -> Option<Cow<str>> {


As with above, please put the lifetime back. Leaving it off the implementations is fine.

BurntSushi · 2017-06-01T12:04:33Z

tests/api.rs

@@ -1,36 +1,48 @@
 #[test]
+#[cfg_attr(not(feature = "cargo-clippy"), allow(unknown_lints))]
+#[allow(trivial_regex, string_lit_as_bytes)]


No, I'm sorry, I cannot abide peppering attributes everywhere for Clippy. These have to go. If that means we don't get clean Clippy output, then we'll just have to live with that.

lukaslueg · 2017-06-01T17:42:14Z

Don't hesitate to let me know of needed changes, it's just a few hunk-jumps :-)

BurntSushi

@lukaslueg Thanks so much! This is looking much better. :-) Unfortunately, I spotted one more nit that I didn't see last time. (I suspect it was accidental.)

BurntSushi · 2017-06-02T00:44:01Z

src/lib.rs

@@ -432,9 +432,9 @@ These classes are based on the definitions provided in
 <pre class="rust">
 \d     digit (\p{Nd})
 \D     not digit
-\s     whitespace (\p{White_Space})
+\s     whitespace (\p{`White_Space`})


Oh hmm, I don't think you should be using back ticks here.

BurntSushi · 2017-06-02T00:44:06Z

src/lib.rs

 \S     not whitespace
-\w     word character (\p{Alphabetic} + \p{M} + \d + \p{Pc} + \p{Join_Control})
+\w     word character (\p{Alphabetic} + \p{M} + \d + \p{Pc} + \p{`Join_Control`})


Or here. :-)

BurntSushi · 2017-06-02T10:49:11Z

@bors r+

bors · 2017-06-02T10:49:12Z

📌 Commit fbb2ba7 has been approved by BurntSushi

bors · 2017-06-02T10:49:18Z

⌛ Testing commit fbb2ba7 with merge 8ffd85d...

Fix clippy warnings This fixes all clippy warnings, mostly lifetime elisions and useless borrows. Clippy now passes on nightly; on stable/without clippy, no lint warnings are given. All tests still pass. I've not moved any code around, which is the reason for allowing `string_lit_as_bytes` in so many places. Up for review

bors · 2017-06-02T11:52:39Z

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing 8ffd85d to master...

Fix clippy warnings

94f8213

lukaslueg commented Jun 1, 2017

View reviewed changes

BurntSushi requested changes Jun 1, 2017

View reviewed changes

Revert some unwarranted clippy-changes

264ef3f

BurntSushi requested changes Jun 2, 2017

View reviewed changes

Revert unwarranted backticks in docs

fbb2ba7

bors merged commit fbb2ba7 into rust-lang:master Jun 2, 2017

Fix clippy warnings #376

Fix clippy warnings #376

Uh oh!

Conversation

lukaslueg commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaslueg commented Jun 1, 2017

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BurntSushi commented Jun 2, 2017

Uh oh!

bors commented Jun 2, 2017

Uh oh!

bors commented Jun 2, 2017

Uh oh!

bors commented Jun 2, 2017

Uh oh!

Uh oh!