Skip to content

Conversation

@lucasloisp
Copy link
Contributor

The lint now checks cases like y != true, as suggested by the issue #3438.

Other cases talked about in the issue (i.e. x < y vs !x && y) are not implemented by this PR.

I can do so if you prefer that the whole issue is closed at once but I might need a little guidance (I will give it a try meanwhile).

Please let me know of any changes necessary

}
} else if let ExprKind::Binary(
Spanned {
node: BinOpKind::Ne, ..
Copy link
Contributor

Choose a reason for hiding this comment

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

since the only difference is the type of operation and some error messages, can you try to deduplicate this code with the above code? Mayne by moving it out into a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking, considering the difference in suggesting !hint or hint, on passing a closure (Fn(Sugg) -> Sugg) such as |hint| !hint.

Do you think there is a more idiomatic way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there is a more idiomatic way of doing this?

Nope, that sounds good to me if that solves all the duplication.

@lucasloisp lucasloisp force-pushed the additional-bool-comparisons branch 3 times, most recently from 31cfea5 to 3964b08 Compare November 30, 2018 21:33
@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 2, 2018
e: &'tcx Expr,
true_message: &str,
false_message: &str,
true_hint: F,
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit: remove the generic parameters and replace their uses in the argument list with impl FnOnce(Sugg<'_>) -> Sugg<'_>

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2018

r=me with nit fixed

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 3, 2018
The lint now checks cases like `y != true`
@lucasloisp lucasloisp force-pushed the additional-bool-comparisons branch from 3964b08 to 3930148 Compare December 3, 2018 19:34
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 4, 2018
@phansch
Copy link
Contributor

phansch commented Dec 4, 2018

LGTM, thanks!

@phansch phansch merged commit 68bb900 into rust-lang:master Dec 4, 2018
@lucasloisp lucasloisp deleted the additional-bool-comparisons branch December 4, 2018 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants