Bad Comparison of Upcasted Ints Fix #42 #802
Conversation
|
While implementing this I noticed that I was duplicating a lot of existing behavior in rustc, especially concerning the function to get bounds for integers. However, that fn is private in the rust lib itself-- is there a standard best practice for handling these cases? |
|
Hm... anybody know what that failure is? It doesn't happen on my machine. |
|
The error on travis is unrelated and only appears on the last nightly version. I’m looking into it. |
| @@ -67,6 +67,7 @@ name | |||
| [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` | |||
| [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases | |||
| [invalid_regex](https://github.com/Manishearth/rust-clippy/wiki#invalid_regex) | deny | finds invalid regular expressions in `Regex::new(_)` invocations | |||
| [invalid_upcast_comparisons](https://github.com/Manishearth/rust-clippy/wiki#invalid_upcast_comparisons) | warn | a comparison involving an term's upcasting to be within the range of the other side of the term is always true or false | |||
There was a problem hiding this comment.
Should be “an term”. That‘s a complicated sentence I had to read several times :-°. As a short description, maybe “a comparison involving an upcast which is always true or false” would do.
There was a problem hiding this comment.
Yes-- I struggled with how to phrase it concisely.
|
Looks good expect for the small nits. I’ll look at the Travis error tomorrow, apparently it’s 1AM already and I have no idea how to fix it 😓 |
| Rel::Le => nlb.0 > norm_rhs_val, | ||
| } { | ||
| // Expression is always false | ||
| cx.span_lint(INVALID_UPCAST_COMPARISONS, |
There was a problem hiding this comment.
Also, I forgot about that yesterday, don’t use cx.span* but one of our span_*. They add a link to Clippy’s wiki.
|
@mcarton Any progress on the Travis error? |
|
It’s been fixed by @Manishearth, but you know have a conflict. You’ll have to rebase with master. |
|
Done. |
| Usize(Us32(x)) => FullInt::U(x as u64), | ||
| Usize(Us64(x)) | | ||
| U64(x) => FullInt::U(x), | ||
| Infer(x) => FullInt::U(x as u64), |
There was a problem hiding this comment.
You could use the erase_type method to only have two kinds to handle here.
|
😕 Git beginner? 😛 |
|
I had conflicts when I did the rebase, so it made me resolve them partway through the rebase (which I think is where the duplicate commits came from). I just ran |
|
@mcarton Anything else you want fixed? |
| if let Some(nlb) = lhs_bounds { | ||
| if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) { | ||
| if rel == Rel::Eq || rel == Rel::Ne { | ||
| if norm_rhs_val < nlb.0 || norm_rhs_val > nlb.0 { |
There was a problem hiding this comment.
There was also a small mistake here, it should be norm_rhs_val > nlb.1. I’ve noticed that while adding more tests. Tests are good 😄
There was a problem hiding this comment.
Thanks! I appreciate your help.
Fix #42.