Skip to content
This repository was archived by the owner on Mar 23, 2021. It is now read-only.

Enable more clippy lints #304

Closed
thomaseizinger opened this issue Sep 28, 2018 · 5 comments · Fixed by #625
Closed

Enable more clippy lints #304

thomaseizinger opened this issue Sep 28, 2018 · 5 comments · Fixed by #625
Assignees

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 28, 2018

There are a couple of interesting lints in clippy that are allow by default but might be useful for us:

Wrong integer arithmetic:

Invalid From implementation:

Println statements:

Use of clone for Arcs and Rcs:
- https://rust-lang-nursery.github.io/rust-clippy/master/index.html#clone_on_ref_ptr

@LLFourn
Copy link
Collaborator

LLFourn commented Dec 8, 2018

"open source" added to force discussion.

@D4nte
Copy link
Contributor

D4nte commented Dec 10, 2018

IMHO: agree for opensource.

@D4nte
Copy link
Contributor

D4nte commented Dec 17, 2018

Agreed for all.
Please note, type inference does not work as well with Arc::clone() and hence we may need to deactivate the warning locally in some places.

@D4nte D4nte added groomed and removed icebox labels Dec 17, 2018
@bonomat bonomat self-assigned this Dec 19, 2018
@bonomat
Copy link
Member

bonomat commented Dec 19, 2018

It seems like clippy does not support a proper config file right now and we need to fall back to passing extra lints as arguments ala:
clippy -- -W clippy::clone_on_ref_ptr -D warnings

More to read: rust-lang/rust-clippy#3164

@bonomat
Copy link
Member

bonomat commented Dec 20, 2018

I opt for not using clone_on_ref_ptr as we would need to make too many exceptions. However, we should make it best practice to use Arc::clone over .clone() where possible.

@bonomat bonomat added this to the Sprint 4 🎄🎅🏿 milestone Dec 20, 2018
@ghost ghost added review and removed work-in-progress labels Jan 8, 2019
@mergify mergify bot closed this as completed in #625 Jan 9, 2019
@ghost ghost removed the review label Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants