-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Don't inline integer literals when they overflow - new attempt #123935
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I somewhat prefer the approach from #118659 (as it's not duplicating checks we have elsewhere), but I also see @WaffleLapkin's argument that we shouldn't be producing a wrong value if a lint gets allowed. Since the lint seems to be triggering in expanded code, maybe we could forbid within the formatting macro expansion? |
I agree that #118659 would have been much more elegant if it handled the edge case correctly. But I think this way is still ok because:
I don't think it's a good idea to deliberately ignore a user when they disable a lint. Since integer overflow is not undefined behaviour in Rust, it is likely that a user who deliberately allows overflowing literals wants to observe its effect. We should support such cases. The alternative is to make integer literal overflow a hard error instead of a lint. @m-ou-se's comment indicated that this might be what users expect in the first place, but promoting this lint to a hard error might be a bigger breaking change. Speaking of which, any resolution to this issue will be a breaking change in case anyone is depending on the current behaviour. EDIT: I forgot that the lang team already determined that this is just a bug and should be fixed as normal. Although they spoke about explicitly suffixed literals, but this fix will also make unsuffixed literals larger than |
Yea that makes sense @bors r+ |
Don't inline integer literals when they overflow - new attempt Basically rust-lang#116633 but I implemented the suggested changes. Fixes rust-lang#115423. Fixes rust-lang#116631. This is my first contribution to this repo so please let me know if I'm supposed to change something :)
Don't inline integer literals when they overflow - new attempt Basically rust-lang#116633 but I implemented the suggested changes. Fixes rust-lang#115423. Fixes rust-lang#116631. This is my first contribution to this repo so please let me know if I'm supposed to change something :)
…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#117919 (Introduce perma-unstable `wasm-c-abi` flag) - rust-lang#123406 (Force exhaustion in iter::ArrayChunks::into_remainder) - rust-lang#123752 (Properly handle emojis as literal prefix in macros) - rust-lang#123935 (Don't inline integer literals when they overflow - new attempt) - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc) - rust-lang#124019 (Use raw-dylib for Windows synchronization functions) - rust-lang#124110 (Fix negating `f16` and `f128` constants) - rust-lang#124112 (Fix ICE when there is a non-Unicode entry in the incremental crate directory) - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#123406 (Force exhaustion in iter::ArrayChunks::into_remainder) - rust-lang#123752 (Properly handle emojis as literal prefix in macros) - rust-lang#123935 (Don't inline integer literals when they overflow - new attempt) - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc) - rust-lang#124019 (Use raw-dylib for Windows synchronization functions) - rust-lang#124110 (Fix negating `f16` and `f128` constants) - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123935 - tstsrt:fix-115423, r=oli-obk Don't inline integer literals when they overflow - new attempt Basically rust-lang#116633 but I implemented the suggested changes. Fixes rust-lang#115423. Fixes rust-lang#116631. This is my first contribution to this repo so please let me know if I'm supposed to change something :)
Basically #116633 but I implemented the suggested changes.
Fixes #115423. Fixes #116631.
This is my first contribution to this repo so please let me know if I'm supposed to change something :)