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

Linting classical overflow checks #741

Merged
merged 3 commits into from
Mar 8, 2016

Conversation

martiansideofthemoon
Copy link
Contributor

@Manishearth , @mcarton getting a compile error in cyclomatic_complexity.rs as follows :- https://pastebin.mozilla.org/8862490
Fix #656.

@mcarton
Copy link
Member

mcarton commented Mar 6, 2016

@martiansideofthemoon Update your nightly

@@ -283,6 +285,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
needless_update::NEEDLESS_UPDATE,
no_effect::NO_EFFECT,
open_options::NONSENSICAL_OPEN_OPTIONS,
overflow_check_conditional::OVERFLOW_CHECK_CONDTIONAL,
Copy link
Member

Choose a reason for hiding this comment

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

You’re missing an i here


/// **What it does:** This lint finds classic overflow checks.
///
/// **Why is this bad?** Most classic overflow checks would cause the compiler to panic.
Copy link
Member

Choose a reason for hiding this comment

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

I would reformulate that to insist on the fact that while those are classic in C, they won’t work in Rust. Also point out that Rust does not need them because of the overflowing_* and wrapping_* family of functions.

@martiansideofthemoon
Copy link
Contributor Author

@Manishearth , @mcarton I hope it's okay 😄


/// **What it does:** This lint finds classic overflow checks.
///
/// **Why is this bad?** Most classic C overflow checks will fail in Rust. Users can use functions like overflowing_* and wrapping_* instead.
Copy link
Member

Choose a reason for hiding this comment

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

Can you put overflowing_* and wrapping_* between quotes? Looks better on the wiki.

`overflowing_*` and `wrapping_*`

@mcarton
Copy link
Member

mcarton commented Mar 7, 2016

You need to run python util/update_lints.py to fix the error, you also have an “unused import” warning. Also can you squash your commits together if you know how to?

@llogiq
Copy link
Contributor

llogiq commented Mar 7, 2016

For bonus points, you could also lint underflow checks, e.g a - b > a or a - b > b.

@martiansideofthemoon
Copy link
Contributor Author

@llogiq I hope it's okay 😄

@llogiq
Copy link
Contributor

llogiq commented Mar 7, 2016

Bonus points for you! 😄 My only small nit is that the lint docs could tell that we lint over- and underflow checks. Also perhaps one could unify some parts into a function to avoid duplication, but I'm fine with merging as it is.

@mcarton
Copy link
Member

mcarton commented Mar 7, 2016

As discussed on IRC, don’t merge the PR yet. He‘ll also handle a < a - b not just a - b > a tomorrow.

@llogiq
Copy link
Contributor

llogiq commented Mar 7, 2016

Even better, although those forms are much less common IIRC.

@martiansideofthemoon
Copy link
Contributor Author

@Manishearth , @llogiq , @mcarton I hope it is alright

@llogiq
Copy link
Contributor

llogiq commented Mar 8, 2016

Unfortunately, it fails the cyclomatic_complexity check. You could factor out some common expressions of both check blocks.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 8, 2016

I've looked at the code, it shouldn't trigger cc, I'll investigate, just throw an allow(cyclomatic_complexity) on your function and I'll fix the lint

@martiansideofthemoon
Copy link
Contributor Author

@oli-obk alright, thanks I'll do it in a bit 😄

@martiansideofthemoon
Copy link
Contributor Author

@llogiq , @oli-obk , @mcarton I've written the allow

llogiq added a commit that referenced this pull request Mar 8, 2016
@llogiq llogiq merged commit 08b7931 into rust-lang:master Mar 8, 2016
@llogiq
Copy link
Contributor

llogiq commented Mar 8, 2016

Thanks! Good work 👍

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.

4 participants