llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.68k stars 10.93k forks source link

[Clang-Tidy] Unnecessary `std::trunc` #96636

Open SunBlack opened 6 days ago

SunBlack commented 6 days ago

Came across the std::trunc method, as we had some code which was basically: myClass.setIntValue(std::trunc(myFloat)). This line lead to the warning -Wfloat-conversion. To prevent someone fixes this by myClass.setIntValue(static_cast<int>(std::trunc(myFloat))), instead of myClass.setIntValue(static_cast<int>(myFloat)), it would be nice if there would be a check for this, when the result of std::trunc is passed directly to a integer type.

I don't know how up-to-date the debate is, but if you follow the discussion here, it also seems to be disadvantageous in terms of performance. So maybe the check could be named performance-unnecessary-trunc.

Just to verify there is no relevant difference, I extended the example source from the std::trunc documentation:

#include <cmath>
#include <initializer_list>
#include <iostream>

int main()
{
    const auto data = std::initializer_list<double>
    {
        +2.7, -2.9, +0.7, -0.9, +0.0, 0.0, -INFINITY, +INFINITY, -NAN, +NAN
    };

    std::cout << std::showpos;
    for (double const x : data)
    {
        int truncInt = std::trunc(x);
        int castInt = static_cast<int>(x);
        std::cout << "trunc(" << x << ") => trunc: " << std::trunc(x) << " truncInt: " << truncInt << " castInt: " << castInt << '\n';
    }
}

Results to:

trunc(+2.7) => trunc: +2 truncInt: +2 castInt: +2
trunc(-2.9) => trunc: -2 truncInt: -2 castInt: -2
trunc(+0.7) => trunc: +0 truncInt: +0 castInt: +0
trunc(-0.9) => trunc: -0 truncInt: +0 castInt: +0
trunc(+0) => trunc: +0 truncInt: +0 castInt: +0
trunc(+0) => trunc: +0 truncInt: +0 castInt: +0
trunc(-inf) => trunc: -inf truncInt: -2147483648 castInt: -2147483648
trunc(+inf) => trunc: +inf truncInt: -2147483648 castInt: -2147483648
trunc(-nan) => trunc: -nan truncInt: -2147483648 castInt: -2147483648
trunc(+nan) => trunc: +nan truncInt: -2147483648 castInt: -2147483648

Examples for the check

// should warn
myClass.setIntValue(std::trunc(myFloat));
myClass.setIntValue(static_cast<int>(std::trunc(myFloat)));
int myInt = std::trunc(myFloat);

// should not warn
myClass.setFloatValue(std::trunc(myFloat));
float myFloat = std::trunc(myFloat);
int myInt = std::trunc(myFloat) * 100.0F; // result of std::trunc used in floating point context and not directly cast to an integer type.
llvmbot commented 6 days ago

@llvm/issue-subscribers-clang-tidy

Author: SunBlack (SunBlack)

Came across the [`std::trunc`](https://en.cppreference.com/w/cpp/numeric/math/trunc) method, as we had some code which was basically: `myClass.setIntValue(std::trunc(myFloat))`. This line lead to the warning `-Wfloat-conversion`. To prevent someone fixes this by `myClass.setIntValue(static_cast<int>(std::trunc(myFloat)))`, instead of `myClass.setIntValue(static_cast<int>(myFloat))`, it would be nice if there would be a check for this, when the result of `std::trunc` is passed directly to a integer type. I don't know how up-to-date the debate is, but if you follow the discussion [here](https://stackoverflow.com/questions/20992209/should-i-explicitly-trunc), it also seems to be disadvantageous in terms of performance. So maybe the check could be named `performance-unnecessary-trunc`. Just to verify there is no relevant difference, I extended the example source from the `std::trunc` documentation: ```cpp #include <cmath> #include <initializer_list> #include <iostream> int main() { const auto data = std::initializer_list<double> { +2.7, -2.9, +0.7, -0.9, +0.0, 0.0, -INFINITY, +INFINITY, -NAN, +NAN }; std::cout << std::showpos; for (double const x : data) { int truncInt = std::trunc(x); int castInt = static_cast<int>(x); std::cout << "trunc(" << x << ") => trunc: " << std::trunc(x) << " truncInt: " << truncInt << " castInt: " << castInt << '\n'; } } ``` Results to: ``` trunc(+2.7) => trunc: +2 truncInt: +2 castInt: +2 trunc(-2.9) => trunc: -2 truncInt: -2 castInt: -2 trunc(+0.7) => trunc: +0 truncInt: +0 castInt: +0 trunc(-0.9) => trunc: -0 truncInt: +0 castInt: +0 trunc(+0) => trunc: +0 truncInt: +0 castInt: +0 trunc(+0) => trunc: +0 truncInt: +0 castInt: +0 trunc(-inf) => trunc: -inf truncInt: -2147483648 castInt: -2147483648 trunc(+inf) => trunc: +inf truncInt: -2147483648 castInt: -2147483648 trunc(-nan) => trunc: -nan truncInt: -2147483648 castInt: -2147483648 trunc(+nan) => trunc: +nan truncInt: -2147483648 castInt: -2147483648 ```