-
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
Add a missing backslash to a makefile #11883
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bors
added a commit
that referenced
this pull request
Jan 29, 2014
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jan 11, 2024
improve [`cast_sign_loss`], to skip warning on always positive expressions fixes: rust-lang#11642 changelog: improve [`cast_sign_loss`] to skip warning on always positive expressions Turns out this is change became quite big, and I still can't cover all the cases, like method calls such as `POSITIVE_NUM.mul(POSITIVE_NUM)`, or `NEGATIVE_NUM.div(NEGATIVE_NUM)`... but well, if I do, I'm scared that this will goes forever, so I stopped, unless it needs to be done, lol.
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Feb 27, 2024
Fix sign-handling bugs and false negatives in `cast_sign_loss` **Note: anyone should feel free to move this PR forward, I might not see notifications from reviewers.** changelog: [`cast_sign_loss`]: Fix sign-handling bugs and false negatives This PR fixes some arithmetic bugs and false negatives in PR rust-lang#11883 (and maybe earlier PRs). Cc `@J-ZhengLi` I haven't updated the tests yet. I was hoping for some initial feedback before adding tests to cover the cases listed below. Here are the issues I've attempted to fix: #### `abs()` can return a negative value in release builds Example: ```rust i32::MIN.abs() ``` https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=022d200f9ef6ee72f629c0c9c1af11b8 Docs: https://doc.rust-lang.org/std/primitive.i32.html#method.abs Other overflows that produce negative values could cause false negatives (and underflows could produce false positives), but they're harder to detect. #### Values with uncertain signs can be positive or negative Any number of values with uncertain signs cause the whole expression to have an uncertain sign, because an uncertain sign can be positive or negative. Example (from UI tests): ```rust fn main() { foo(a: i32, b: i32, c: i32) -> u32 { (a * b * c * c) as u32 //~^ ERROR: casting `i32` to `u32` may lose the sign of the value } println!("{}", foo(1, -1, 1)); } ``` https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=165d2e2676ee8343b1b9fe60db32aadd #### Handle `expect()` the same way as `unwrap()` Since we're ignoring `unwrap()` we might as well do the same with `expect()`. This doesn't seem to have tests but I'm happy to add some like `Some(existing_test).unwrap() as u32`. #### A negative base to an odd exponent is guaranteed to be negative An integer `pow()`'s sign is only uncertain when its operants are uncertain. (Ignoring overflow.) Example: ```rust ((-2_i32).pow(3) * -2) as u32 ``` This offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. (Rather than just an odd number of uncertain signs.) #### Both sides of a multiply or divide should be peeled recursively I'm not sure why the lhs was peeled recursively, and the rhs was left intact. But the sign of any sequence of multiplies and divides is determined by the signs of its operands. (Ignoring overflow.) I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable. But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, these should all lint: ```rust fn peel_all(x: i32) { (-p(x) * -p(x) * -p(x)) as u32; ((-p(x) * -p(x)) * -p(x)) as u32; (-p(x) * (-p(x) * -p(x))) as u32; } ``` #### The right hand side of a Rem doesn't change the sign Unlike Mul and Div, > Given remainder = dividend % divisor, the remainder will have the same sign as the dividend. https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable. But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, only the first six expressions should lint. The expressions that start with a constant should lint (or not lint) regardless of whether the lint supports `p()` or unary negation, because only the dividend's sign matters. Example: ```rust fn rem_lhs(x: i32) { (-p(x) % -1) as u32; (-p(x) % 1) as u32; (-1 % -p(x)) as u32; (-1 % p(x)) as u32; (-1 % -x) as u32; (-1 % x) as u32; // These shouldn't lint: (p(x) % -1) as u32; (p(x) % 1) as u32; (1 % -p(x)) as u32; (1 % p(x)) as u32; (1 % -x) as u32; (1 % x) as u32; } ``` #### There's no need to bail on other expressions When peeling, any other operators or expressions can be left intact and sent to the constant evaluator. If these expressions can be evaluated, this offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. If not, they end up marked as having uncertain sign.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #11874