Page MenuHomeWildfire Games

Fix ambiguous uses of abs [-Wabsolute-value]
ClosedPublic

Authored by historic_bruno on Jul 17 2019, 6:26 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22500: Fixes some ambiguous calls of abs().
Trac Tickets
#5515
Summary

There are a handful of ambiguous abs calls in the source:

../../../source/lib/tex/tex_bmp.cpp:106:17: warning: absolute value function 'abs' given an argument of type 'const long' but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value]
        const long h = abs(h_);
                       ^
../../../source/lib/tex/tex_bmp.cpp:106:17: note: use function 'std::abs' instead
        const long h = abs(h_);
                       ^~~
                       std::abs

../../../source/lib/tex/tex_bmp.cpp:106:17: warning: absolute value function 'abs' given an argument of type 'const long' but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value]
        const long h = abs(h_);
                       ^
../../../source/lib/tex/tex_bmp.cpp:106:17: note: use function 'std::abs' instead
        const long h = abs(h_);
                       ^~~
                       std::abs

In file included from ../../../source/simulation2/components/tests/test_HierPathfinder.cpp:17:
/Users/user/GitHub/0ad/source/simulation2/components/tests/test_HierPathfinder.h:440:13: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
                TS_ASSERT(abs(euclidian(goal.x.ToInt_RoundToNegInfinity(), goal.z.ToInt_RoundToNegInfinity(), pi, pj)-20) < 1.5f);
                          ^
/Users/user/GitHub/0ad/source/simulation2/components/tests/test_HierPathfinder.h:440:13: note: use function 'std::abs' instead
                TS_ASSERT(abs(euclidian(goal.x.ToInt_RoundToNegInfinity(), goal.z.ToInt_RoundToNegInfinity(), pi, pj)-20) < 1.5f);
                          ^~~
                          std::abs

In file included from ../../../source/simulation2/components/tests/test_HierPathfinder.cpp:17:
/Users/user/GitHub/0ad/source/simulation2/components/tests/test_HierPathfinder.h:441:13: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
                TS_ASSERT(abs(euclidian(goal.x.ToInt_RoundToNegInfinity(), goal.z.ToInt_RoundToNegInfinity(), oi, oj) - euclidian(pi, pj, oi, oj)) < 22.0f);
                          ^
/Users/user/GitHub/0ad/source/simulation2/components/tests/test_HierPathfinder.h:441:13: note: use function 'std::abs' instead
                TS_ASSERT(abs(euclidian(goal.x.ToInt_RoundToNegInfinity(), goal.z.ToInt_RoundToNegInfinity(), oi, oj) - euclidian(pi, pj, oi, oj)) < 22.0f);
                          ^~~
                          std::abs

The problem is the compiler might select either the C or C++ version of abs. The C version is int-only, while the C++ version has overloads for various types. Building on OS X 10.11 actually warns about these (10.14 doesn't), but I believe the calls aren't well-defined anyway and should be fixed.

Test Plan
  1. Apply diff
  2. Build game
  3. Observe no [-Wabsolute-value] warnings
  4. Tests should pass
  5. Textures should decode properly, in particular:
    • Bitmaps with negative height (stored top-down, not bottom-up)
    • DDS files with DDSD_LINEARSIZE flag set

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

historic_bruno created this revision.Jul 17 2019, 6:26 AM
wraitii added a reviewer: Restricted Owners Package.Jul 17 2019, 10:01 AM
elexis accepted this revision.Jul 17 2019, 12:22 PM
elexis added a subscriber: elexis.

std::fabs, std::labs?

Otherwise compiles, tests working, individual code changes correct.
I didn't get the compile warnings on gcc9, but if the ones you posted above are all you got, then the diff is complete.

This revision is now accepted and ready to land.Jul 17 2019, 12:22 PM
In D2091#87068, @elexis wrote:

std::fabs, std::labs?

I wasn't sure, we mix fabs and std::fabs all over the engine.

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jul 18 2019, 12:39 AM