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

add new lint else_if_without_else #2298

Merged
merged 1 commit into from
Jan 17, 2018

Conversation

goodmanjonathan
Copy link
Contributor

@goodmanjonathan goodmanjonathan commented Dec 24, 2017

This lint checks that every if expression containing an else if has a terminating else block. If expressions without else ifs and/or elses are not linted.

cc #2227

@goodmanjonathan
Copy link
Contributor Author

compiletest-rs failed to build (Manishearth/compiletest-rs#93)

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

lgtm, just some doc nits

/// **What it does:** Checks for usage of if expressions with an else if branch,
/// but without a final `else` branch.
///
/// **Why is this bad?** Some coding guidelines require this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reference the MISRA standard rule here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

/// } else if x.is_negative() {
/// b()
/// } else {
/// // we don't care about zero
Copy link
Contributor

Choose a reason for hiding this comment

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

zero is positive I think :D
just throw an unreachable!() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, {integer}.is_positive() returns false for 0, which is what I had in mind.

/// // we don't care about zero
/// }
/// ```
declare_lint! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it a restriction lint. I don't think this should be part of the pedantic group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@goodmanjonathan goodmanjonathan force-pushed the else_if_without_else branch 4 times, most recently from 91e82ea to f6f370e Compare January 14, 2018 21:55
@goodmanjonathan
Copy link
Contributor Author

Addressed comments (except for x.is_positive(), as noted earlier).

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2018

retriggering travis

@oli-obk oli-obk closed this Jan 16, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2018

retriggering travis

@oli-obk oli-obk reopened this Jan 16, 2018
@oli-obk oli-obk merged commit a2fdfc0 into rust-lang:master Jan 17, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2018

Thanks!

@goodmanjonathan
Copy link
Contributor Author

Thanks for the review!

@goodmanjonathan goodmanjonathan deleted the else_if_without_else branch January 18, 2018 00:15
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