Bug 163869 - GCC warning in testb3.cpp testAbsArgWithEffectfulDoubleConversion
Summary: GCC warning in testb3.cpp testAbsArgWithEffectfulDoubleConversion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-23 08:26 PDT by Michael Catanzaro
Modified: 2016-10-24 06:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.90 KB, patch)
2016-10-23 08:58 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-10-23 08:26:17 PDT
Since r207696 "Expand Trunc in B3 to support Double to Float":

[284/3418] Building CXX object Source/...akeFiles/testb3.dir/__/b3/testb3.cpp.o
../../Source/JavaScriptCore/b3/testb3.cpp: In function ‘void {anonymous}::testAbsArgWithEffectfulDoubleConversion(float)’:
../../Source/JavaScriptCore/b3/testb3.cpp:3832:38: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
     CHECK(isIdentical(effect, fabs(a)));
                                      ^
../../Source/JavaScriptCore/b3/testb3.cpp:100:16: note: in definition of macro ‘CHECK’
         if (!!(x))                                                      \
                ^
In file included from ../../Source/JavaScriptCore/b3/air/AirArg.h:31:0,
                 from ../../Source/JavaScriptCore/b3/air/AirCode.h:30,
                 from ../../Source/JavaScriptCore/b3/testb3.cpp:28:
../../Source/JavaScriptCore/b3/B3Common.h:67:13: note: candidate 1: bool JSC::B3::isIdentical(double, double)
 inline bool isIdentical(double left, double right)
             ^~~~~~~~~~~
../../Source/JavaScriptCore/b3/B3Common.h:72:13: note: candidate 2: bool JSC::B3::isIdentical(float, float)
 inline bool isIdentical(float left, float right)
             ^~~~~~~~~~~
../../Source/JavaScriptCore/b3/testb3.cpp: In function ‘void {anonymous}::testSqrtArgWithEffectfulDoubleConversion(float)’:
../../Source/JavaScriptCore/b3/testb3.cpp:4336:38: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
     CHECK(isIdentical(effect, sqrt(a)));
                                      ^
../../Source/JavaScriptCore/b3/testb3.cpp:100:16: note: in definition of macro ‘CHECK’
         if (!!(x))                                                      \
                ^
In file included from ../../Source/JavaScriptCore/b3/air/AirArg.h:31:0,
                 from ../../Source/JavaScriptCore/b3/air/AirCode.h:30,
                 from ../../Source/JavaScriptCore/b3/testb3.cpp:28:
../../Source/JavaScriptCore/b3/B3Common.h:67:13: note: candidate 1: bool JSC::B3::isIdentical(double, double)
 inline bool isIdentical(double left, double right)
             ^~~~~~~~~~~
../../Source/JavaScriptCore/b3/B3Common.h:72:13: note: candidate 2: bool JSC::B3::isIdentical(float, float)
 inline bool isIdentical(float left, float right)
             ^~~~~~~~~~~
Comment 1 Michael Catanzaro 2016-10-23 08:42:59 PDT
Hm, I'm not sure how to fix this.

I tried turning all the different definitions of isIdentical into template specializations, but that just added a bunch of new ambiguous function call build failures.
Comment 2 Michael Catanzaro 2016-10-23 08:52:50 PDT
Ah OK, the problem is the code is somehow accidentally using std::fabs and std::sqrt (not sure how) and getting a float result, but it looks like the test actually wants ::fabs and ::sqrt so that the return value will be a double.

Alternatively, the return values could just be cast to double to fix the warning, but not sure if that would be right for this test or not.
Comment 3 Michael Catanzaro 2016-10-23 08:55:34 PDT
(In reply to comment #2)
> Ah OK, the problem is the code is somehow accidentally using std::fabs and
> std::sqrt (not sure how) and getting a float result, but it looks like the
> test actually wants ::fabs and ::sqrt so that the return value will be a
> double.

Hm, I'm wrong, it doesn't silence the warning....

> Alternatively, the return values could just be cast to double to fix the
> warning, but not sure if that would be right for this test or not.

I think this should be fine.
Comment 4 Michael Catanzaro 2016-10-23 08:58:37 PDT
Created attachment 292546 [details]
Patch
Comment 5 Michael Catanzaro 2016-10-24 06:20:43 PDT
Looks like Zan committed an identical patch today.