Suggest hypot for x.mul_add(x, y * y).sqrt() - #17600
Conversation
rustbot
added
the
S-waiting-on-community-reviews
|
Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews. In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews. |
rustbot
added
the
S-waiting-on-review
|
These are not equivalent mathematically; they differ by the presence/absence of a square root at the end. |
There was a problem hiding this comment.
Community review:
If you can clear up my sqrt confusion regarding why this is linting on this despite it not having any check for it, this LGTM.
I also would like to see a bit more testing since I think this lint might be hiding some things that are wrong, but my intuition might also be wrong here 😉
1e07cf7 to
bfc86ca
Compare
| if let ExprKind::MethodCall(PathSegment { ident: method, .. }, self_arg, [arg1, arg2], _) = receiver.kind | ||
| && method.name == sym::mul_add | ||
| && eq_expr_value(cx, ctxt, self_arg, arg1) | ||
| && let ExprKind::Binary( | ||
| Spanned { | ||
| node: BinOpKind::Mul, .. | ||
| }, | ||
| mul_lhs, | ||
| mul_rhs, | ||
| ) = arg2.kind | ||
| && eq_expr_value(cx, ctxt, mul_lhs, mul_rhs) |
There was a problem hiding this comment.
I'm not actually sure if we typically expect more effort than this when checking that we're dealing with the right method, but from what I've found this seems to be pretty close to our usual "best effort".
That said, maybe it'd be nice to double check that all operands involved are the same type, and that the type in question is a floating point? Just for a little extra peace of mind. I think you can find some inspiration in clippy_lints/src/floating_point_arithmetic/expm1.rs.
There was a problem hiding this comment.
Yes why not, but I believe testing that self_arg here is a floating point would be enough, as rustc would lock in place its specific mul_add implementation (f64 or f32... for both operands in each case).
There was a problem hiding this comment.
I took a swing at breaking it as of 9acfaae and you seem to be right. Thanks!
rustbot
removed
the
S-waiting-on-community-reviews
|
r? @Manishearth rustbot has assigned @Manishearth for the project review. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Gri-ffin
changed the title
hypot for x.mul_add(x, y * y)hypot for x.mul_add(x, y * y).sqrt()
6 participants
changelog: [
imprecise_flops]: also suggesthypotfor the formx.mul_add(x, y * y).sqrt()