Fix some clippy lints up to rust 1.41.1 #780

marmeladema · 2021-05-15T13:14:21Z

Some lints have been intentionally ignored, especially:

any lints that would change public APIs (like &self -> self)
any lints that would introduce new public APIs (like impl Default over fn new())

It seems in some cases, struct initialization with redundant field: field was on purpose if one field (at least) was not redundant. This PR does "simplify" those cases, but that might not be wanted?

BurntSushi

Thanks for doing this! It looks like the vast majority of the changes are in struct initialization. Those LGTM. Specifically:

It seems in some cases, struct initialization with redundant field: field was on purpose if one field (at least) was not redundant. This PR does "simplify" those cases, but that might not be wanted?

A lot of the code in this crate was written before the short-hand field initialization syntax was added, and I just never saw a good reason to go through and update everything. (And some of the code was even written before Rust 1.0.) So thank you for doing that. :-)

But there are a few changes I don't agree with. It's generally why I don't run Clippy and why it's not hooked up to CI either. I find it to be too opinionated and just haven't bothered to go through the list of lints to whitelist only the ones I agree with. And even if I agree with a lint generally, I don't always agree with its universal application. And that in turn requires allow'ing lints within certain scopes which I find a bit annoying. So that's kind of another reason why I haven't started using Clippy.

If you can fix up some of the nits I had, then I'd be happy to merge this. Thanks again!

regex-syntax/src/ast/parse.rs

regex-syntax/src/hir/interval.rs

regex-syntax/src/hir/translate.rs

regex-syntax/src/unicode_tables/mod.rs

BurntSushi · 2021-05-15T16:55:51Z

src/compile.rs

@@ -387,11 +388,11 @@ impl Compiler {
        } else {
            let entry = self.insts.len();
            let hole = self.push_hole(InstHole::Save { slot: first_slot });
-            let patch = self.c(expr)?.unwrap_or(self.next_inst());
+            let patch = self.c(expr)?.unwrap_or_else(|| self.next_inst());


I guess these unwrap_or_elses are fine, although, I suspect they don't really make a difference when it comes to perf, so I feel like this is another case of making the code more obtuse for no real reason.

Since the same kind of pattern (.unwrap_or_else(|| self.next_inst()))) is repeated at least 4 times, it might be better to introduce a dedicated function? I'll try to look into it

Possibly.

To be honest, this whole compiler is going to be thrown away "soon" as part of #656. The current state of that rewrite (not quite to parity yet) is here: https://github.com/BurntSushi/regex-automata/blob/c4fee38d7499721a2eccaa7074f21bf2c955cbce/src/nfa/thompson/compiler.rs

The rewrite is faster for the cases I've tested, generates more compact bytecode and is overall much clearer. (It does away with the whole InstHole thing.)

So I think it's probably not wise to spend too much time refactoring or clarifying the code here. :-) With that said, a helper function sounds fine if you want to pursue that.

I have kept the unwrap_or_else for those cases, I hope that's ok!

src/utf8.rs

marmeladema · 2021-05-15T17:39:15Z

But there are a few changes I don't agree with. It's generally why I don't run Clippy and why it's not hooked up to CI either. I find it to be too opinionated and just haven't bothered to go through the list of lints to whitelist only the ones I agree with. And even if I agree with a lint generally, I don't always agree with its universal application. And that in turn requires allow'ing lints within certain scopes which I find a bit annoying. So that's kind of another reason why I haven't started using Clippy.

I fully agree with that statement, however it does find useful things sometime, so running it manually from time to time can be valuable.

If you can fix up some of the nits I had, then I'd be happy to merge this. Thanks again!

I will make the requested change 👍

Another change I had in mind is to annotate the error enums which contain a __NonExhaustive variant with #[non_exhaustive] attribute (introduced by rust 1.40.0 so before 1.41.1). However, even though that variant is hidden from documentation, I guess this is technically a breaking change. But I tried and it did find an occurrence where one valid variant was not handled properly so it might be beneficial. What do you think?

BurntSushi · 2021-05-15T20:45:17Z

I fully agree with that statement, however it does find useful things sometime, so running it manually from time to time can be valuable.

Definitely agree there. I should do this more often.

Another change I had in mind is to annotate the error enums which contain a __NonExhaustive variant with #[non_exhaustive] attribute (introduced by rust 1.40.0 so before 1.41.1). However, even though that variant is hidden from documentation, I guess this is technically a breaking change. But I tried and it did find an occurrence where one valid variant was not handled properly so it might be beneficial. What do you think?

I believe that, in principle, yes we should move to #[non_exhaustive] and I believe it provides the desired semantics. The __NonExhaustive variant is not part of the stable API, so removing that and replacing it with #[non_exhaustive] is fine to do is a semver compatible release. With that said, if it causes too much breakage, I'm a practical person: I would bring back that variant, yank the old release and put out a new one. But I don't think it will. (When doing these sorts of things in std, we usually do a crater run and see what happens.)

Some lints have been intentionally ignored, especially: * any lints that would change public APIs (like &self -> self) * any lints that would introduce new public APIs (like Default over new)

Some lints have been intentionally ignored, especially: * any lints that would change public APIs (like &self -> self) * any lints that would introduce new public APIs (like Default over new) Closes #780

marmeladema force-pushed the fix-clippy-lints branch from dca881d to 31f64cd Compare May 15, 2021 13:17

BurntSushi requested changes May 15, 2021

View reviewed changes

marmeladema force-pushed the fix-clippy-lints branch from 31f64cd to 2d133a4 Compare May 17, 2021 19:09

marmeladema added 2 commits May 17, 2021 20:10

Fix clippy lints up to rust 1.41.1 in regex-syntax crate

a42d7cc

Some lints have been intentionally ignored, especially: * any lints that would change public APIs (like &self -> self) * any lints that would introduce new public APIs (like Default over new)

Fix clippy lints up to rust 1.41.1 in main regex crate

105761d

Some lints have been intentionally ignored, especially: * any lints that would change public APIs (like &self -> self) * any lints that would introduce new public APIs (like Default over new)

marmeladema force-pushed the fix-clippy-lints branch from 2d133a4 to 105761d Compare May 17, 2021 19:10

marmeladema requested a review from BurntSushi May 17, 2021 20:04

BurntSushi closed this in 9ca3099 Jul 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some clippy lints up to rust 1.41.1 #780

Fix some clippy lints up to rust 1.41.1 #780

marmeladema commented May 15, 2021

BurntSushi left a comment

BurntSushi May 15, 2021

marmeladema May 15, 2021

BurntSushi May 15, 2021

marmeladema May 17, 2021

marmeladema commented May 15, 2021

BurntSushi commented May 15, 2021

Fix some clippy lints up to rust 1.41.1 #780

Fix some clippy lints up to rust 1.41.1 #780

Conversation

marmeladema commented May 15, 2021

BurntSushi left a comment

Choose a reason for hiding this comment

BurntSushi May 15, 2021

Choose a reason for hiding this comment

marmeladema May 15, 2021

Choose a reason for hiding this comment

BurntSushi May 15, 2021

Choose a reason for hiding this comment

marmeladema May 17, 2021

Choose a reason for hiding this comment

marmeladema commented May 15, 2021

BurntSushi commented May 15, 2021