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

ci: enable clippy lints #1335

Merged
merged 25 commits into from
Jul 25, 2019
Merged

ci: enable clippy lints #1335

merged 25 commits into from
Jul 25, 2019

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jul 21, 2019

Related: #1334

Those that may change the public API and Those that may change the implementation significantly is allowed (trivially_copy_pass_by_ref, mut_from_ref, etc.).

Refs: https://rust-lang.github.io/rust-clippy/master/index.html (list of clippy lints)

cc @carllerche

* clippy::mut_from_ref
* clippy::map_clone
* clippy::should_implement_trait
* clippy::transmute_ptr_to_ptr
* clippy::trivially_copy_pass_by_ref
* clippy::type_complexity
* clippy::module_inception
* clippy::wrong_self_convention
* clippy::mutex_atomic
* clippy::needless_lifetimes
* clippy::unnecessary_mut_passed
* clippy::assign_op_pattern
* clippy::while_let_on_iterator
* clippy::let_and_return
* clippy::needless_range_loop
* clippy::len_zero
* clippy::single_match
* clippy::match_bool
* clippy::redundant_pattern_matching
* clippy::needless_return
* clippy::collapsible_if
* clippy::let_unit_value
* clippy::unit_arg
* clippy::duration_subsec
* clippy::redundant_closure
* clippy::new_without_default
* clippy::option_map_unit_fn
* clippy::cast_lossless
@taiki-e taiki-e closed this Jul 21, 2019
@taiki-e taiki-e reopened this Jul 21, 2019
@taiki-e
Copy link
Member Author

taiki-e commented Jul 21, 2019

Seems rustfmt somehow checks the crates in /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/, but I'm not sure about how to fix or reproduce it:
https://dev.azure.com/tokio-rs/Tokio/_build/results?buildId=1569&view=logs&jobId=f14073de-e1a4-5ba6-5dc1-4c667fb92951

@carllerche
Copy link
Member

@taiki-e that is very odd... adding clippy does this?

@carllerche
Copy link
Member

Does this happen on master?

@taiki-e
Copy link
Member Author

taiki-e commented Jul 21, 2019

Currently, I think eb4d80a (bump rustc version to nightly-2019-07-19) is related because rustfmt error did not occur in 9761168.

However, no problems occurred in locally with either branch.

@taiki-e
Copy link
Member Author

taiki-e commented Jul 21, 2019

Does this happen on master?

However, no problems occurred in locally with either branch.

I opened #1338 to check it.

@taiki-e taiki-e requested a review from carllerche July 21, 2019 21:45
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good 👍

I added thoughts inline. Also, could we avoid file level allows in favor of adding them on the item? I'm not sure what they are referencing. thanks!

tokio-reactor/src/lib.rs Outdated Show resolved Hide resolved
tokio-sync/src/lib.rs Outdated Show resolved Hide resolved
tokio-sync/src/semaphore.rs Outdated Show resolved Hide resolved
tokio-test/src/task.rs Outdated Show resolved Hide resolved
tokio-threadpool/src/config.rs Outdated Show resolved Hide resolved
tokio-threadpool/src/lib.rs Outdated Show resolved Hide resolved
tokio-timer/src/clock/mod.rs Outdated Show resolved Hide resolved
tokio-timer/src/timer/entry.rs Outdated Show resolved Hide resolved
tokio-udp/src/lib.rs Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@ pub struct MockTask {
waker: Arc<ThreadWaker>,
}

#[allow(clippy::mutex_atomic)]
Copy link
Member

Choose a reason for hiding this comment

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

I recommended removing this lint: rust-lang/rust-clippy#4295

Copy link
Member

Choose a reason for hiding this comment

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

We probably should just disable this lint at the project level. This can happen in this PR or follow up work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in ae3bd60.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks 👍

I think most of the allow bits (that are not false positives) can be removed. That can happen here or later. I approved, so feel free to merge if you are happy.

pub fn leave_multicast_v4(&self, multiaddr: &Ipv4Addr, interface: &Ipv4Addr) -> io::Result<()> {
self.io.get_ref().leave_multicast_v4(multiaddr, interface)
pub fn leave_multicast_v4(&self, multiaddr: Ipv4Addr, interface: Ipv4Addr) -> io::Result<()> {
self.io.get_ref().leave_multicast_v4(&multiaddr, &interface)
Copy link
Member Author

@taiki-e taiki-e Jul 24, 2019

Choose a reason for hiding this comment

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

mio 0.7 changed this to pass by value 👍 : tokio-rs/mio#894

@taiki-e
Copy link
Member Author

taiki-e commented Jul 24, 2019

@carllerche I have removed all of allow(clippy::..) that are not false positives. (They were much easier than I had imagined. I thought it would cause other warnings but it did not happen.)

@taiki-e
Copy link
Member Author

taiki-e commented Jul 25, 2019

Resolved a conflict, will merge once CI passed.

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