Bug 109325

Summary: The 'global isinf/isnan' compiler quirk required when using clang with libstdc++
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, allan.jensen, andersca, benjamin, cmarcelo, dgrogan, dino, d-r, eric.carlson, eric, feature-media-reviews, fmalita, gns, haraken, japhet, jsbell, macpherson, menard, mrobinson, ojan.autocc, pdr, pnormand, schenney, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109314    
Attachments:
Description Flags
Patch
none
Patch andersca: review+

Description Zan Dobersek 2013-02-08 14:02:41 PST
The 'global isinf/isnan' compiler quirk required when using clang with libstdc++
Comment 1 Zan Dobersek 2013-02-08 14:14:01 PST
Created attachment 187363 [details]
Patch
Comment 2 Anders Carlsson 2013-02-10 09:17:18 PST
Comment on attachment 187363 [details]
Patch

Why can't we always use std::isinf and std::isnan instead of doing this?
Comment 3 Zan Dobersek 2013-02-11 03:00:45 PST
The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.

It's more of a standard library quirk than a compiler quirk, though.
Comment 4 Anders Carlsson 2013-02-11 10:16:50 PST
(In reply to comment #3)
> The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> 
> It's more of a standard library quirk than a compiler quirk, though.

That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?
Comment 5 Allan Sandfeld Jensen 2013-02-11 10:34:01 PST
(In reply to comment #4)
> (In reply to comment #3)
> > The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> > 
> > It's more of a standard library quirk than a compiler quirk, though.
> 
> That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?

Yes exactly, and previously the coding style required us to use 'using namespace std;'. This has changed now, but we  might still have problems with some compilers not having the functions in the std namespace (seems the Intel compiler only fixed that recently).
Comment 6 Anders Carlsson 2013-02-11 10:39:42 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> > > 
> > > It's more of a standard library quirk than a compiler quirk, though.
> > 
> > That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?
> 
> Yes exactly, and previously the coding style required us to use 'using namespace std;'. This has changed now, but we  might still have problems with some compilers not having the functions in the std namespace (seems the Intel compiler only fixed that recently).

In that case, I'd rather see us move in that direction (adding explicit std:: qualifications for isinf and isnan). If some STL versions don't put isinf and isnan in the std namespace than we should add workarounds for that.
Comment 7 Allan Sandfeld Jensen 2013-02-11 10:58:59 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> > > > 
> > > > It's more of a standard library quirk than a compiler quirk, though.
> > > 
> > > That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?
> > 
> > Yes exactly, and previously the coding style required us to use 'using namespace std;'. This has changed now, but we  might still have problems with some compilers not having the functions in the std namespace (seems the Intel compiler only fixed that recently).
> 
> In that case, I'd rather see us move in that direction (adding explicit std:: qualifications for isinf and isnan). If some STL versions don't put isinf and isnan in the std namespace than we should add workarounds for that.

I agree we should move to std:: prefixed methods everywhere, but I am not sure it should be done here and now.

I have not seen or heard of any issues with this quirk except that it looks kind of ugly, but it is localised to a few lines of code in a single file, and as long as it doesn't cause any trouble, I personally think it would be a wrong to undo it, if doing so could potentially create new issues to an unknown set of other configurations.

For this patch in particular, I would accept it as long as it only affects it own configuration of clang+glibcpp.
Comment 8 Anders Carlsson 2013-02-11 13:19:50 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> > > > > 
> > > > > It's more of a standard library quirk than a compiler quirk, though.
> > > > 
> > > > That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?
> > > 
> > > Yes exactly, and previously the coding style required us to use 'using namespace std;'. This has changed now, but we  might still have problems with some compilers not having the functions in the std namespace (seems the Intel compiler only fixed that recently).
> > 
> > In that case, I'd rather see us move in that direction (adding explicit std:: qualifications for isinf and isnan). If some STL versions don't put isinf and isnan in the std namespace than we should add workarounds for that.
> 
> I agree we should move to std:: prefixed methods everywhere, but I am not sure it should be done here and now.
> 
> I have not seen or heard of any issues with this quirk except that it looks kind of ugly, but it is localised to a few lines of code in a single file, and as long as it doesn't cause any trouble, I personally think it would be a wrong to undo it, if doing so could potentially create new issues to an unknown set of other configurations.
> 

Again, I'd much rather see a patch that just replaces all calls to isinf/isnan with the prefixed versions. I'd be willing to write one myself.
Comment 9 Zan Dobersek 2013-02-11 13:46:30 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (In reply to comment #3)
> > > > > > The quirk was introduced in bug #59249 to work around the collision between C99 and C++11 isnan and isinf methods.
> > > > > > 
> > > > > > It's more of a standard library quirk than a compiler quirk, though.
> > > > > 
> > > > > That still doesn't answer my question. I'm guessing that there's an ambiguity error when calling isinf or isnan when the std namespace has been brought in using "using namespace std"?
> > > > 
> > > > Yes exactly, and previously the coding style required us to use 'using namespace std;'. This has changed now, but we  might still have problems with some compilers not having the functions in the std namespace (seems the Intel compiler only fixed that recently).
> > > 
> > > In that case, I'd rather see us move in that direction (adding explicit std:: qualifications for isinf and isnan). If some STL versions don't put isinf and isnan in the std namespace than we should add workarounds for that.
> > 
> > I agree we should move to std:: prefixed methods everywhere, but I am not sure it should be done here and now.
> > 
> > I have not seen or heard of any issues with this quirk except that it looks kind of ugly, but it is localised to a few lines of code in a single file, and as long as it doesn't cause any trouble, I personally think it would be a wrong to undo it, if doing so could potentially create new issues to an unknown set of other configurations.
> > 
> 
> Again, I'd much rather see a patch that just replaces all calls to isinf/isnan with the prefixed versions. I'd be willing to write one myself.

I can work on that as well, for starters modifying calls to isinf/isnan by prefixing them. I could then revisit the need for this specific compiler quirk both in general case of using gcc with libstdc++ and the case of using clang with libstdc++.

If I'm understanding the stretch goals correctly here all the calls to other methods in the std namespace would then also move to using the std:: prefix, which I might fancy doing as well.
Comment 10 Anders Carlsson 2013-02-11 13:54:07 PST
> 
> I can work on that as well, for starters modifying calls to isinf/isnan by prefixing them. 

That sounds great!

> I could then revisit the need for this specific compiler quirk both in general case of using gcc with libstdc++ and the case of using clang with libstdc++.

Sounds good.

> If I'm understanding the stretch goals correctly here all the calls to other methods in the std namespace would then also move to using the std:: prefix, which I might fancy doing as well.

I think that's a separate patch though.
Comment 11 Allan Sandfeld Jensen 2013-02-11 14:10:51 PST
(In reply to comment #9)
> I can work on that as well, for starters modifying calls to isinf/isnan by prefixing them. I could then revisit the need for this specific compiler quirk both in general case of using gcc with libstdc++ and the case of using clang with libstdc++.
> 
To avoid issues with unprefixed isinf and isnan from popping up here and there in future, it may also be worthwhile to make the style-checker warn against it.
Comment 12 Zan Dobersek 2013-02-13 04:26:01 PST
Created attachment 188054 [details]
Patch
Comment 13 WebKit Review Bot 2013-02-13 04:39:42 PST
Attachment 188054 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSValueRef.cpp', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JSCTypedArrayStubs.h', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/offlineasm/cloop.rb', u'Source/JavaScriptCore/runtime/DateConstructor.cpp', u'Source/JavaScriptCore/runtime/DateInstance.cpp', u'Source/JavaScriptCore/runtime/DatePrototype.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.cpp', u'Source/JavaScriptCore/runtime/JSDateMath.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp', u'Source/JavaScriptCore/runtime/MathObject.cpp', u'Source/JavaScriptCore/runtime/PropertyDescriptor.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Compiler.h', u'Source/WTF/wtf/DateMath.cpp', u'Source/WTF/wtf/IntegralTypedArrayBase.h', u'Source/WTF/wtf/MathExtras.h', u'Source/WTF/wtf/MediaTime.cpp', u'Source/WTF/wtf/Uint8ClampedArray.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp', u'Source/WebCore/Modules/webaudio/AudioParam.cpp', u'Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp', u'Source/WebCore/Modules/webaudio/PannerNode.cpp', u'Source/WebCore/bindings/js/IDBBindingUtilities.cpp', u'Source/WebCore/bindings/js/JSDataViewCustom.cpp', u'Source/WebCore/bindings/js/JSGeolocationCustom.cpp', u'Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp', u'Source/WebCore/bindings/js/JSWebKitPointCustom.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp', u'Source/WebCore/bindings/v8/IDBBindingUtilities.cpp', u'Source/WebCore/bindings/v8/V8Binding.cpp', u'Source/WebCore/bindings/v8/custom/V8GeolocationCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WebKitPointCustom.cpp', u'Source/WebCore/bridge/qt/qt_runtime.cpp', u'Source/WebCore/css/WebKitCSSMatrix.cpp', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/MediaController.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/track/TextTrack.cpp', u'Source/WebCore/html/track/TextTrackCue.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/WindowFeatures.cpp', u'Source/WebCore/platform/CalculationValue.cpp', u'Source/WebCore/platform/Decimal.cpp', u'Source/WebCore/platform/Length.cpp', u'Source/WebCore/platform/audio/AudioResampler.cpp', u'Source/WebCore/platform/audio/DynamicsCompressorKernel.cpp', u'Source/WebCore/platform/audio/Reverb.cpp', u'Source/WebCore/platform/graphics/Font.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/gpu/LoopBlinnMathUtils.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm', u'Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp', u'Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp', u'Source/WebCore/platform/network/ResourceResponseBase.cpp', u'Source/WebCore/rendering/RenderMediaControlsChromium.cpp', u'Source/WebCore/rendering/RenderThemeMac.mm', u'Source/WebCore/svg/SVGAnimationElement.cpp', u'Source/WebCore/svg/SVGSVGElement.cpp', u'Source/WebCore/svg/animation/SMILTime.h', u'Source/WebCore/svg/animation/SVGSMILElement.cpp', u'Source/WebCore/xml/XPathFunctions.cpp', u'Source/WebCore/xml/XPathValue.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/tests/DecimalTest.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebView.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestRunner.cpp']" exit_code: 1
Source/WTF/wtf/MathExtras.h:206:  wtf_fmod is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Zan Dobersek 2013-02-13 13:16:34 PST
(In reply to comment #11)
> (In reply to comment #9)
> > I can work on that as well, for starters modifying calls to isinf/isnan by prefixing them. I could then revisit the need for this specific compiler quirk both in general case of using gcc with libstdc++ and the case of using clang with libstdc++.
> > 
> To avoid issues with unprefixed isinf and isnan from popping up here and there in future, it may also be worthwhile to make the style-checker warn against it.

Created bug #109729.
Comment 15 Zan Dobersek 2013-02-13 15:02:55 PST
Committed r142810: <http://trac.webkit.org/changeset/142810>