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

Bad suggestion for overflowing hex literal in signed operation #53628

Open
tspiteri opened this issue Aug 23, 2018 · 5 comments
Open

Bad suggestion for overflowing hex literal in signed operation #53628

tspiteri opened this issue Aug 23, 2018 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tspiteri
Copy link
Contributor

Code:

pub fn flip_msb(a: i32) -> i32 {
    a ^ 0x8000_0000
}

Diagnostic:

warning: literal out of range for i32
 --> src/lib.rs:2:9
  |
2 |     a ^ 0x8000_0000
  |         ^^^^^^^^^^^
  |
  = note: #[warn(overflowing_literals)] on by default
  = note: the literal `0x8000_0000` (decimal `2147483648`) does not fit into an `i32` and will become `-2147483648i32`
  = help: consider using `u32` instead

In this case using u32 is not an option (you could use a u32 literal and cast to i32, but that's not ideal). A better suggestion in this case would be to use -0x8000_0000 instead.

This issue is, I think, more relevant to hex (or other non-decimal) representations, as for example format!("{:#x}", i32::MIN) would produce 0x80000000, which could lead someone to think that that is a valid warning-free i32 literal.

@csmoe csmoe added the A-diagnostics Area: Messages for errors, warnings, and lints label Aug 23, 2018
@hellow554
Copy link
Contributor

hellow554 commented Aug 23, 2018

Hmm, while you are right, I don't like the expression -0x8000_0000 at all (but who am I, that my opinion matters ;) ).
For me, 0x8000_0000 is already negative (because the MSB is set), so what does -0x8000_0000 even mean?
I think, the suggestion is already pretty darn good (use a u32 instead)

@tspiteri
Copy link
Contributor Author

tspiteri commented Aug 23, 2018

My suggestion was to do the wrapping and output the suggestion using the same radix as the original. As another example, for a ^ 0xc000_0000 the suggestion would be -0x4000_0000 (or maybe just -0x40000000 as it is harder to keep the underscores). So picking the edge case 0x8000_0000 made things unnecessarily unclear.

(For the compiler, the hex literal for the minimum i32 is -0x80000000, and 0x8000_0000 is an overflowing literal not a negative number. I don't think changing the language is an option here.)

@estebank
Copy link
Contributor

You can write the following:

#[allow(overflowing_literals)] // 0x8000_0000 is MIN_I32
pub fn flip_msb(a: i32) -> i32 {
    a ^ 0x8000_0000
}

@steveklabnik
Copy link
Member

Triage: this is now an error, not a warning, but the diagnostic has not changed.

@AaronKutch
Copy link
Contributor

AaronKutch commented Feb 23, 2023

No, the actual problem here is that -i32::MAX != i32::MIN, the maximum possible value of i32 is 0x7fffffff not 0x80000000. If you are doing bit manipulation, what you should do is convert to u32 first which has no confusion associated with it, or you could do a ^ -0x80000000i32 (edit: whoops I did that calculation wrong, this is why you should always cast to the unsigned types first for bit manip)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants