Bug 116352 - REGRESSION (r149184): Build errors in RefPtr.h when building with Clang, C++98 standard
Summary: REGRESSION (r149184): Build errors in RefPtr.h when building with Clang, C++9...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-17 14:31 PDT by Zan Dobersek
Modified: 2013-06-18 00:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.15 KB, patch)
2013-06-17 07:52 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-05-17 14:31:05 PDT
r149184 introduced move semantics for the RefPtr, conditioning them with the compiler's rvalue references support.
http://trac.webkit.org/changeset/149184

The implementation uses the std::move method, supported only in C++11. Still, the Clang compiler can support rvalue references even in C++98 mode due to the extension that is provided and detected via the __has_extension macro in Compiler.h[1]. The __has_extension macro is used intentionally instead of __has_feature, the switch was made in r119162.
http://trac.webkit.org/changeset/119162

This causes compilation errors due to undeclared std::move when building JSC (which AFAIK nobody builds in C++11 std), using Clang 3.2, libstdc++ on the GTK port. Like said, with this configuration, the rvalue references support is enabled, but the C++11 standard is not, ergo the failures.

Same errors are occurring in WTFString.cpp, where std::move is used in String::isolatedCopy when reference-qualified functions support is present. Again the __has_extension macro is used, so the support is enabled even when not building in C++11 std, resulting in build errors due to undeclared std::move. That change was introduced in r149372.
http://trac.webkit.org/changeset/149372

I've verified that with the given configuration, using __has_feature instead of __has_extension fixes the problem, but I'm not sure if that's OK given the change log in r119132 which specifically switched to __has_extension for some Clang features so they could be used even when using the C++98 standard.
http://trac.webkit.org/changeset/119132

[1] http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h#L56
[2] http://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/WTFString.cpp#L657
Comment 1 Anders Carlsson 2013-05-17 15:35:07 PDT
Iā€™m fine with changing it; on Mac we build all of WebKit with C++11.
Comment 2 Zan Dobersek 2013-05-18 11:57:15 PDT
Would it be OK to replace all the uses of __has_extension with __has_feature?

This would basically enforce to build either in strict C++98 or strict C++11 standard mode, with no extensions provided by Clang for C++98 taken into account.
Comment 3 Sam Weinig 2013-05-19 13:20:36 PDT
(In reply to comment #2)
> Would it be OK to replace all the uses of __has_extension with __has_feature?
> 
> This would basically enforce to build either in strict C++98 or strict C++11 standard mode, with no extensions provided by Clang for C++98 taken into account.

Why is anyone building with Clang and C++98.  I think we should strive for Clang plus at least C++11.
Comment 4 Zan Dobersek 2013-05-20 04:29:08 PDT
That would work for the GTK port due to limiting the supported compilers. I'm not sure how ambitious other ports are regarding Clang support.

Then again, if C++11 is made mandatory when using Clang, using __has_extension doesn't have much meaning at the moment since these features are part of C++11 so __has_feature should cover proper detection.
Comment 5 Zan Dobersek 2013-06-17 07:52:23 PDT
Created attachment 204823 [details]
Patch
Comment 6 Zan Dobersek 2013-06-18 00:00:34 PDT
Comment on attachment 204823 [details]
Patch

Clearing flags on attachment: 204823

Committed r151670: <http://trac.webkit.org/changeset/151670>
Comment 7 Zan Dobersek 2013-06-18 00:00:41 PDT
All reviewed patches have been landed.  Closing bug.