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

Too many clippy warnings #913

Open
vihdzp opened this issue Jun 16, 2021 · 2 comments
Open

Too many clippy warnings #913

vihdzp opened this issue Jun 16, 2021 · 2 comments

Comments

@vihdzp
Copy link
Contributor

vihdzp commented Jun 16, 2021

Currently, running cargo clippy on nalgebra yields 581 warnings.

A few of these are false positives, and some others are just complaints about the complicated types of nalgebra (for which not much can be done). However, many of these should be strictly good improvements to the code. Many of these fixes would also be breaking changes, so we should be aware of that.

I believe that all of these warnings should either be fixed or turned off. That way, if any future sub-par code props up, we'll quickly be able to detect it and fix it.

@vihdzp vihdzp mentioned this issue Jun 16, 2021
@sebcrozet
Copy link
Member

sebcrozet commented Jun 17, 2021

Yeah, nalgebra started being developed before clippy existed, and I never actually used it since then unfortunately. There have been some efforts to fix clippy warning on other merged PRs but obviously we haven't fixed everything yet.

@Andlon
Copy link
Collaborator

Andlon commented Jun 17, 2021

I think this is a really worthwhile endeavor. Once we have gotten to the "0 baseline" (either by fixing problems or judiciously ignoring individual special cases where we are doing the right thing), we can also consider adding stricter lints on top.

For example, we recently had a case where an automatic derive of Deserialize caused an unsoundness hole due to invariants used in unsafe code not being upheld. It turns out that there's a clippy lint (not turned on by default) that would have caught this issue, see rustsec/advisory-db#880 for more information.

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

No branches or pull requests

3 participants