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

mul_add struggling with long expressions #4735

Open
chrisduerr opened this issue Oct 26, 2019 · 0 comments
Open

mul_add struggling with long expressions #4735

chrisduerr opened this issue Oct 26, 2019 · 0 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@chrisduerr
Copy link
Contributor

I've recently gotten the clippy lint for mul_add and ran into it with a cubic bezier calculation.

Currently the function looks like this:

(1.0 - x).powi(3) * p0
    + 3.0 * (1.0 - x).powi(2) * x * p1
    + 3.0 * (1.0 - x) * x.powi(2) * p2
    + x.powi(3) * p3

The initial lint for this function alone looks something like this:

warning: consider using mul_add() for better numerical precision
  --> src/main.rs:27:5
   |
27 | /     (1.0 - x).powi(3) * p0
28 | |         + 3.0 * (1.0 - x).powi(2) * x * p1
29 | |         + 3.0 * (1.0 - x) * x.powi(2) * p2
30 | |         + x.powi(3) * p3
   | |________________________^
   |
   = note: `#[warn(clippy::manual_mul_add)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_mul_add
help: try
   |
27 |     x.powi(3).mul_add(p3, (1.0 - x).powi(3) * p0
28 |         + 3.0 * (1.0 - x).powi(2) * x * p1
29 |         + 3.0 * (1.0 - x) * x.powi(2) * p2)
   |

warning: consider using mul_add() for better numerical precision
  --> src/main.rs:27:5
   |
27 | /     (1.0 - x).powi(3) * p0
28 | |         + 3.0 * (1.0 - x).powi(2) * x * p1
29 | |         + 3.0 * (1.0 - x) * x.powi(2) * p2
   | |__________________________________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_mul_add
help: try
   |
27 |     3.0 * (1.0 - x) * x.powi(2).mul_add(p2, (1.0 - x).powi(3) * p0
28 |         + 3.0 * (1.0 - x).powi(2) * x * p1)
   |

warning: consider using mul_add() for better numerical precision
  --> src/main.rs:27:5
   |
27 | /     (1.0 - x).powi(3) * p0
28 | |         + 3.0 * (1.0 - x).powi(2) * x * p1
   | |__________________________________________^ help: try: `(1.0 - x).powi(3).mul_add(p0, 3.0 * (1.0 - x).powi(2) * x * p1)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_mul_add

warning: consider using mul_add() for better numerical precision
  --> src/main.rs:27:5
   |
27 | /     (1.0 - x).powi(3) * p0
28 | |         + 3.0 * (1.0 - x).powi(2) * x * p1
   | |__________________________________________^ help: try: `3.0 * (1.0 - x).powi(2) * x.mul_add(p1, (1.0 - x).powi(3) * p0)`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_mul_add

    Finished dev [unoptimized + debuginfo] target(s) in 0.00s

Since there is a lot of multiplication and adding going on, the output is quite a bit to take in. However, following its suggestions can lead to incorrect code.

The first expression correctly recognizes the whole thing as a single expression (potentially by pure coincidence) and suggests a change that does not break things, since it respects the order of mathematical operations.

However, when looking at more of the output, it seems like the later suggestions only look at part of the operation. Especially when both multiplication and addition is involved at the same time on multiple levels, this can be dangerous. If I look at the output in my terminal and start at the bottom with the last thing output first, then I'll break the code.

The following code would be the result of applying the last suggestion:

// (1.0 - x).powi(3) * p0
//     + 3.0 * (1.0 - x).powi(2) * x * p1
3.0 * (1.0 - x).powi(2) * x.mul_add(p1, (1.0 - x).powi(3) * p0)
//
    + 3.0 * (1.0 - x) * x.powi(2) * p2
    + x.powi(3) * p3

The issue with this is that performing a x.mul_add first, means that the other multiplications are done after the addition. With a single parenthesis, which fixes the suggestion, the issue is probably explained best:

// 3.0 * (1.0 - x).powi(2) * x.mul_add(p1, (1.0 - x).powi(3) * p0)
  (3.0 * (1.0 - x).powi(2) * x).mul_add(p1, (1.0 - x).powi(3) * p0)
    + 3.0 * (1.0 - x) * x.powi(2) * p2
    + x.powi(3) * p3
@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Oct 26, 2019
bors added a commit that referenced this issue Oct 26, 2019
Move manual_mul_add into nursery

Addresses #4735

changelog: Move [`manual_mul_add`] into nursery
@phansch phansch added I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied and removed I-false-positive Issue: The lint was triggered on code it shouldn't have labels Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

No branches or pull requests

3 participants