Bug 86465 – GCC 4.7 and C++11

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Allan Sandfeld Jensen]

WebKit should be buildable using gcc-4.7 in C++11 mode. Also we should take advantage of the available C++11 features we already have support for.

[Allan Sandfeld Jensen]

Created attachment 141919 [details]
Patch

[WebKit Review Bot]

Attachment 141919 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/platform/network/ResourceResponseBase.cpp:36:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Source/WebCore/html/InputType.cpp:81:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Source/WebCore/html/InputType.cpp:82:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/bindings/js/JSGeolocationCustom.cpp:41:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/html/NumberInputType.cpp:51:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:43:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebKit2/WebProcess/WebPage/FindController.cpp:44:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Total errors found: 7 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> Attachment 141919 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> Source/WebCore/platform/network/ResourceResponseBase.cpp:36:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
> Source/WebCore/html/InputType.cpp:81:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
> Source/WebCore/html/InputType.cpp:82:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
> Source/WebCore/bindings/js/JSGeolocationCustom.cpp:41:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
> Source/WebCore/html/NumberInputType.cpp:51:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:43:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
> Source/WebKit2/WebProcess/WebPage/FindController.cpp:44:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
> Total errors found: 7 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.
They are not false positive, the true negatives. The style checker is not just checking something pointless, it is advising harmful behaviour and advising against correct behaviour.

[Allan Sandfeld Jensen]

Created attachment 141931 [details]
Patch for check-webkit-style

Experimental patch to fix check-webkit-style if the above patch goes through.

[Allan Sandfeld Jensen]

Created attachment 142275 [details]
Patch

[WebKit Review Bot]

Attachment 142275 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WTF/wtf/MathExtras.h:292:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/MathExtras.h:298:  Use 'using namespace std;' instead of 'using std::wtf_isinf;'.  [build/using_std] [4]
Source/WTF/wtf/MathExtras.h:299:  Use 'using namespace std;' instead of 'using std::wtf_isnan;'.  [build/using_std] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 142275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142275&action=review

Too many different fixes that resolve various different issues all in one patch. I understand that all are driven by compatibility with the compiler, but if there are any problems it would be better to have separate patches; doing it all at once makes it more likely someone will roll the whole thing out.

> Source/WTF/wtf/Compiler.h:132
> +#define WTF_COMPILER_SUPPORTS_GCC_ISINF_ISNAN_QUIRK 1

I'm not sure it makes sense to say that a compiler “supports a quirk”. I think this is a library bug, not a compiler quirk.

> Source/WTF/wtf/Noncopyable.h:42
>              ClassName& operator=(const ClassName&) = delete; \
>          _Pragma("clang diagnostic pop")
>  #else
> +#define WTF_MAKE_NONCOPYABLE(ClassName) \
> +        private: \
> +            ClassName(const ClassName&) = delete; \
> +            ClassName& operator=(const ClassName&) = delete;
> +#endif
> +#else

The cleaner way to make this change is to make the clang diagnostic push/pop/ignore thing into a separate macro, but I suppose this is OK for now.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:313
> -    RunLoop::main()->dispatch(bind(&NetscapePlugin::handlePluginThreadAsyncCall, this, function, userData));
> +    RunLoop::main()->dispatch(::bind(&NetscapePlugin::handlePluginThreadAsyncCall, this, function, userData));

I think this should be WTF::bind, although ::bind will work. I also think we should talk to Anders about a better solution to this conflict between WTF::bind and std::bind.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:159
> -    m_webPage->drawingArea()->dispatchAfterEnsuringUpdatedScrollPosition(bind(&FindController::updateFindUIAfterPageScroll, this, found, string, options, maxMatchCount));
> +    m_webPage->drawingArea()->dispatchAfterEnsuringUpdatedScrollPosition(::bind(&FindController::updateFindUIAfterPageScroll, this, found, string, options, maxMatchCount));

Ditto.

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (From update of attachment 142275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142275&action=review
> 
> Too many different fixes that resolve various different issues all in one patch. I understand that all are driven by compatibility with the compiler, but if there are any problems it would be better to have separate patches; doing it all at once makes it more likely someone will roll the whole thing out.
> 
Sounds like a good plan, one the issues already have a separate bug, but if I split the rest, should I open separate bugs, or just link them all to this one?

> > Source/WTF/wtf/Compiler.h:132
> > +#define WTF_COMPILER_SUPPORTS_GCC_ISINF_ISNAN_QUIRK 1
> 
> I'm not sure it makes sense to say that a compiler “supports a quirk”. I think this is a library bug, not a compiler quirk.
> 
True, I also thought it sounded odd. Perhaps I could rename it WTF_COMPILER_QUIRK_GLOBAL_ISINF_ISNAN_CONFLICT. If we find more compiler/standard library issues in the future, we could at some point use a COMPILER_QUIRK macro.

> > Source/WTF/wtf/Noncopyable.h:42
> >              ClassName& operator=(const ClassName&) = delete; \
> >          _Pragma("clang diagnostic pop")
> >  #else
> > +#define WTF_MAKE_NONCOPYABLE(ClassName) \
> > +        private: \
> > +            ClassName(const ClassName&) = delete; \
> > +            ClassName& operator=(const ClassName&) = delete;
> > +#endif
> > +#else
> 
> The cleaner way to make this change is to make the clang diagnostic push/pop/ignore thing into a separate macro, but I suppose this is OK for now.
> 
Yeah, I will have another look when I separate it into a separate patch.

> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:313
> > -    RunLoop::main()->dispatch(bind(&NetscapePlugin::handlePluginThreadAsyncCall, this, function, userData));
> > +    RunLoop::main()->dispatch(::bind(&NetscapePlugin::handlePluginThreadAsyncCall, this, function, userData));
> 
> I think this should be WTF::bind, although ::bind will work. I also think we should talk to Anders about a better solution to this conflict between WTF::bind and std::bind.
> 
I agree. I will rename it WTF::bind unless we come up with a completely different solution.

[Allan Sandfeld Jensen]

Created attachment 142986 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 142987 [details]
Patch2

[Allan Sandfeld Jensen]

Created attachment 142997 [details]
Patch3

[Allan Sandfeld Jensen]

Created attachment 143000 [details]
Patch4

[WebKit Review Bot]

Comment on attachment 142986 [details]
Patch

Rejecting attachment 142986 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Hunk #3 succeeded at 77 (offset -3 lines).
Hunk #4 FAILED at 118.
Hunk #5 succeeded at 222 (offset -10 lines).
1 out of 5 hunks FAILED -- saving rejects to file Source/WTF/wtf/Compiler.h.rej
patching file Source/WTF/wtf/Noncopyable.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/qmake/mkspecs/features/unix/default_post.prf

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12728921

[WebKit Review Bot]

Comment on attachment 142997 [details]
Patch3

Clearing flags on attachment: 142997

Committed r117777: <http://trac.webkit.org/changeset/117777>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

All patches are not landed

[WebKit Review Bot]

Comment on attachment 142986 [details]
Patch

Rejecting attachment 142986 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Hunk #3 succeeded at 77 (offset -3 lines).
Hunk #4 FAILED at 118.
Hunk #5 succeeded at 222 (offset -10 lines).
1 out of 5 hunks FAILED -- saving rejects to file Source/WTF/wtf/Compiler.h.rej
patching file Source/WTF/wtf/Noncopyable.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/qmake/mkspecs/features/unix/default_post.prf

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12744205

[WebKit Review Bot]

Comment on attachment 142987 [details]
Patch2

Clearing flags on attachment: 142987

Committed r117782: <http://trac.webkit.org/changeset/117782>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

No, not all reviewed patches have been landed.

[Allan Sandfeld Jensen]

Comment on attachment 142986 [details]
Patch

Making for review to avoid the bot closing the bug again, and to remind us it needs pushing.

[WebKit Review Bot]

Comment on attachment 143000 [details]
Patch4

Clearing flags on attachment: 143000

Committed r117798: <http://trac.webkit.org/changeset/117798>

[Simon Hausmann]

Comment on attachment 142986 [details]
Patch

Clearing flags on attachment: 142986

Committed r117934: <http://trac.webkit.org/changeset/117934>

[Simon Hausmann]

All reviewed patches have been landed.  Closing bug.