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 some warnings #680

Merged
merged 1 commit into from Aug 10, 2021
Merged

Fix some warnings #680

merged 1 commit into from Aug 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 7, 2021

No description provided.

@mulkieran
Copy link
Member

@remilauzier What tool+version is giving you these warnings?

@mulkieran mulkieran assigned ghost Aug 7, 2021
@ghost
Copy link
Author

ghost commented Aug 7, 2021

It's rust_2018_idioms with rust stable. Hidden anonymous lifetime are deprecated and will be removed in the 2021 version. The remove u32 is IntelliJ IDEA. Using std:u32 are deprecated too. Using directly u32 is the new way.

@mulkieran
Copy link
Member

It's rust_2018_idioms with rust stable. Hidden anonymous lifetime are deprecated and will be removed in the 2021 version. The remove u32 is IntelliJ IDEA. Using std:u32 are deprecated too. Using directly u32 is the new way.

Ok! In our Makefile we have a variable RUST_2018_IDIOMS. Could you add the other lints in that category that this PR fixes to that list?

@mulkieran
Copy link
Member

@remilauzier I noticed that you edited lib.rs w/ a bunch of deny directives. Is that so that Intellij IDE and possibly other IDEs can read that info and act appropriately? I use a text editor for all my development, and haven't been following the evolution of IDEs in the last couple years. If that is really the case, then we might consider moving our management of compiler and clippy errors out of the Makefile and into the Rust source entirely. Please let me know, thx.

@ghost
Copy link
Author

ghost commented Aug 9, 2021

It's been a few year that rustc and clippy read directly deny warning from the source file. That permit everyone to know what to do or not easily. It's really make to not depend on a single IDE. You can even put a #[allow(something)] directly for a single function if you want. You can even tell clippy to not let you see some warning if you put the msrv in clippy.toml. Doing it in lib.rs or main.rs permit it to apply for all the project. I must said it's a first for me to have these in a makefile.

@mulkieran
Copy link
Member

@remilauzier This project has been around since before the "?" operator existed and when clippy could only be run on nightly! But, on reading around a bit, it looks like there are benefits to keeping these directives in a Makefile. Here's one pretty good reason: https://www.reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/. And another: rust-lang/cargo#5998. We already use a clippy.toml to set the msrv....that seems to have come in handy. But we'll stick with setting those clippy and compiler warning directives in the Makefile, for the foreseeable future.

Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

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

Ok, thx for you answer. After some thought, I've decided to go w/ my original review. At some point, though, I'll probably try to move the two remaining directives in lib.rs into the Makefile as well.

src/lib.rs Outdated
@@ -62,6 +62,14 @@
//! 6. Optionally loop and re-invoke `poll()` on the fd to wait for more
//! events.

#![deny(
Copy link
Member

Choose a reason for hiding this comment

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

Please drop this edit and put any additional compiler restrictions you can add in the Makefile. We use a DENY variable globally in all the targets, essentially. If you can augment that in any way, carry on. Also, since it looks like you can make this pass all the rust-2018-idioms lints, you can just delete the RUST_2018_IDIOMS variable entirely and put -D rust-2018-idioms in the value of the DENY variable.

You'll notice that the two directives we have in lib.rs are very non-technical and not related to Rust itself. They went in very early to both encourage documentation of methods and to make it easy to write meaningful comments w/out having to worry about formatting.

@mulkieran mulkieran requested a review from jbaublitz August 9, 2021 16:25
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.

2 participants