Bug 109325 – The 'global isinf/isnan' compiler quirk required when using clang with libstdc++

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

The 'global isinf/isnan' compiler quirk required when using clang with libstdc++

[Zan Dobersek]

Created attachment 187363 [details]
Patch

[Anders Carlsson]

Comment on attachment 187363 [details]
Patch

Why can't we always use std::isinf and std::isnan instead of doing this?

[Zan Dobersek]

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.

[Anders Carlsson]

(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"?

[Allan Sandfeld Jensen]

(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).

[Anders Carlsson]

(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.

[Allan Sandfeld Jensen]

(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.

[Anders Carlsson]

(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.

[Zan Dobersek]

(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.

[Anders Carlsson]

> 
> 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.

[Allan Sandfeld Jensen]

(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.

[Zan Dobersek]

Created attachment 188054 [details]
Patch

[WebKit Review Bot]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Committed r142810: <http://trac.webkit.org/changeset/142810>