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

lint: add extra integer lints, and partially fix some code #3409

Merged
merged 5 commits into from
Jan 27, 2022
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jan 26, 2022

Motivation

I thought we might be able to add some lints for risky integer conversions in PR #3401. But it turns out that code is actually needed for now. (Until we do #2211.)

But I thought I should submit the lints that did work.

Solution

  • add some integer and pointer clippy lints that pass on our code
  • partially fix an integer calculation lint

Review

Anyone can review this Rust and lint code, it's not urgent or important.

Reviewer Checklist

  • CI passes with new lints
  • Existing tests pass

Follow Up Work

We might want to enable some of these other lints eventually, but it's a very low priority.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement P-Optional ✨ A-network Area: Network protocol updates or fixes A-diagnostics Area: Diagnosing issues or monitoring performance labels Jan 26, 2022
@teor2345 teor2345 self-assigned this Jan 26, 2022
.cargo/config.toml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #3409 (6d650b7) into main (499ae89) will decrease coverage by 0.03%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #3409      +/-   ##
==========================================
- Coverage   78.34%   78.30%   -0.04%     
==========================================
  Files         267      267              
  Lines       31526    31529       +3     
==========================================
- Hits        24698    24690       -8     
- Misses       6828     6839      +11     

@teor2345 teor2345 changed the title Int lints lint: add extra integer lints, and partially fix some code Jan 26, 2022
@mpguerra mpguerra requested a review from oxarbitrage January 27, 2022 14:14
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this is great, not only added more lints for integers but also suggests how to add more. The more we can have is def better. I am unsure if we should open a ticket with that TODO list or if the code text is enough.

@oxarbitrage oxarbitrage merged commit 4f0d7bd into main Jan 27, 2022
@oxarbitrage oxarbitrage deleted the int-lints branch January 27, 2022 14:34
@teor2345
Copy link
Contributor Author

I think this is great, not only added more lints for integers but also suggests how to add more. The more we can have is def better. I am unsure if we should open a ticket with that TODO list or if the code text is enough.

I don't think we will add any more lints any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-diagnostics Area: Diagnosing issues or monitoring performance A-network Area: Network protocol updates or fixes C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants