-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Special case overflowing literal lints to warn about -iX::MIN
#108383
Comments
I think that is already roughly what the compiler suggests. For example, this code: #[allow(unused_variables)]
fn main() {
let a = 5;
let b = a ^ 0x80000000i32;
} results in the following diagnostic:
Maybe you want an additional note there about the pitfalls of using "edge" values like |
The compiler didn't actually do that when the issue was filed. The So given that we have these suggestions now, would it make sense to close this issue? |
The first new note solves half the issue cases I think. There are cases where people might not understand the asymmetry between MIN and MAX, it is not immediately obvious why removing an unary negative causes these errors unless you know about the edge case. I think one extra note should be added just for the iX::MIN case that says with hexadecimal, "note: iX::MIN == -0x80..., but iX::MAX == 0x7ff..., meaning that positive 0x80... is not representable as an iX." with the 'X' and '...' properly filled in. |
Makes sense |
I just found several issues where people forget that
-i32::MAX != i32::MIN
,i32::MAX == 7fffffff
whilei32::MIN == -0x80000000
. People see that-0x80000000
works and then mistakenly try to use+0x80000000
which doesn't work becausei32
can't represent it. What we should actually do is detect when people try to type out0x80000000i32
or0x80i8
, etc, and have a lint that warns about this problem. I'm thinking the lint should suggest casting to the unsigned type and back, because the user is probably trying to do bit manipulation which has subtleties with signed integers.#53628, #99195, #108269
The text was updated successfully, but these errors were encountered: