Bug 134500 – Add WTF::move()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Daniel Bates]

As seen in bug #134304 and <http://trac.webkit.org/changeset/170594>, using std::move() is error prone. In particular, std::move() is happy to take a const lvalue reference as its argument even though we won't be allowed to move such a type (*). Ideally the compiler would warn us of such bad usage.

(*) The return value of std::move() for such an argument may allow for the invocation of a move constructor or move-assignment operator taking a const rvalue reference, but such functions are nonsensical (since we're talking about transferring ownership when talking about move operations).

[Daniel Bates]

Created attachment 234177 [details]
Patch

Add WTF::move(), which compile asserts that its argument is a non-const lvalue reference that is movable before calling through to std::move().

[Daniel Bates]

Assuming we choose to land such an idiom I'll send out an email to webkit-dev to announce and evangelize it before landing the patch.

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/text/AtomicString.h:81:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/RuleSet.h:131:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/MainThread.cpp:217:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/Font.cpp:218:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:236:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:869:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:879:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:889:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp:85:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit2/Scripts/webkit2/LegacyMessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/LegacyMessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/MessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/MessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:34:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/xml/XPathValue.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/xml/XPathValue.h:82:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/AccessorCallJITStubRoutine.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 17 in 688 files


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

[Daniel Bates]

Created attachment 234211 [details]
Patch

From my understanding from reading section 14.8.2/8 [temp.deduct] C++ standard (*), GCC and MSVC seem to treat access control checks during template deduction as compile-time errors instead of a deduction failure.

(*) <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf>

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/text/AtomicString.h:81:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/RuleSet.h:131:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/MainThread.cpp:217:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/Font.cpp:218:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:236:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:869:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:879:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:889:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp:85:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit2/Scripts/webkit2/LegacyMessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/LegacyMessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/MessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit2/MessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:34:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/xml/XPathValue.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/xml/XPathValue.h:82:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/AccessorCallJITStubRoutine.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 17 in 688 files


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

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Created an attachment (id=234211) [details]
> Patch
> 
> From my understanding from reading section 14.8.2/8 [temp.deduct] C++ standard (*), GCC and MSVC seem to treat access control checks during template deduction as compile-time errors instead of a deduction failure.
> 
> (*) <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf>

Please also file a bug on <http://llvm.org/bugs/>.

[Daniel Bates]

Does anyone know what version of GCC the rego-gtk-ews GTK-WK2 bot is using? Is it 4.8?

CC'ing Manuel Rego Casasnovas, who I suspect manages the rego-gtk-ews bot.

[Darin Adler]

(In reply to comment #6)
> Please also file a bug on <http://llvm.org/bugs/>.

I think Dan is saying that clang gets this right and gcc and MSVC get it wrong. So I don’t think we need to file a clang bug report. But maybe I’m misunderstanding.

[Daniel Bates]

(In reply to comment #8)
> (In reply to comment #6)
> > Please also file a bug on <http://llvm.org/bugs/>.
> 
> I think Dan is saying that clang gets this right and gcc and MSVC get it wrong. So I don’t think we need to file a clang bug report. But maybe I’m misunderstanding.

That's correct. I claim that this is a bug in GCC and MSVC and that Clang behaves correctly. I'll look to file bugs against GCC and MSVC.

[Daniel Bates]

Comment on attachment 234211 [details]
Patch

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

> Source/WTF/wtf/StdLibExtras.h:130
> +#if COMPILER(CLANG)

After talking with Darin Adler yesterday (07/01), we should assume we're using a compiler that conforms to the C++11 standard. For now, GCC and MSVC don't conform to it with respect to the handling of access control checks during template deduction.

I'll change this line to have the form:

#if COMPILER(MSVC) || COMPILER(GCC) && !COMPILER(CLANG)

before landing this patch

[Zan Dobersek]

(In reply to comment #7)
> Does anyone know what version of GCC the rego-gtk-ews GTK-WK2 bot is using? Is it 4.8?
> 
> CC'ing Manuel Rego Casasnovas, who I suspect manages the rego-gtk-ews bot.

It uses GCC 4.8 by the looks of libstdc++ include paths.

I'll test this out locally with GCC 4.7 through 4.9.

[Manuel Rego Casasnovas]

(In reply to comment #7)
> Does anyone know what version of GCC the rego-gtk-ews GTK-WK2 bot is using? Is it 4.8?
> 
> CC'ing Manuel Rego Casasnovas, who I suspect manages the rego-gtk-ews bot.

Yeah, the version is:
$ gcc --version
gcc (Debian 4.8.2-16) 4.8.2

BTW, I can try manually any patch in the bot if needed.

[Manuel Rego Casasnovas]

Comment on attachment 234211 [details]
Patch

FWIW, this builds fine in 4.9.0 (my bot is using that version now).

[Zan Dobersek]

(In reply to comment #13)
> (From update of attachment 234211 [details])
> FWIW, this builds fine in 4.9.0 (my bot is using that version now).

As told on IRC, Rego still used the build guard that only enables the move-constructible and move-assignable checks on Clang.

So there's indeed a problem with all GCC versions, and guarding out the std::is_move_constructible<> and std::is_move_assignable<> uses (as it's done in the second patch) avoids the compilation errors.

[Daniel Bates]

Created attachment 234284 [details]
Patch 1 of 2 - Only adds WTF::move()

As requested on IRC by Anders Carlsson, a patch that only adds WTF::move().

[Daniel Bates]

(In reply to comment #10)
> (From update of attachment 234211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234211&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:130
> > +#if COMPILER(CLANG)
> 
> After talking with Darin Adler yesterday (07/01), we should assume we're using a compiler that conforms to the C++11 standard. For now, GCC and MSVC don't conform to it with respect to the handling of access control checks during template deduction.
> 
> I'll change this line to have the form:
> 
> #if COMPILER(MSVC) || COMPILER(GCC) && !COMPILER(CLANG)

I meant to write:

#if !COMPILER(MSVC) && (!COMPILER(GCC) || COMPILER(CLANG))

[Build Bot]

Comment on attachment 234284 [details]
Patch 1 of 2 - Only adds WTF::move()

Attachment 234284 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4808763638808576

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 234298 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 234284 [details]
Patch 1 of 2 - Only adds WTF::move()

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

> Source/WTF/wtf/StdLibExtras.h:122
> +template<class T>

I prefer typename.

> Source/WTF/wtf/StdLibExtras.h:135
> +    static_assert(std::is_move_constructible<NonConstNonRefQualifiedType>::value || std::is_move_assignable<NonConstNonRefQualifiedType>::value, "T is not movable.");

I thought we discussed that we want && here, not ||.

[Darin Adler]

Comment on attachment 234284 [details]
Patch 1 of 2 - Only adds WTF::move()

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

> Source/WTF/wtf/StdLibExtras.h:123
> +ALWAYS_INLINE typename std::remove_reference<T>::type&& move(T&& value)

To match the model of the rest of WTF, I think we’d want to put a "using WTF::move" at the end of the file. Or maybe we explicitly don’t want that and want to be explicit about using WTF::move at every site where we use it.

I’m not 100% sure that StdLibExtras.h is the perfect place for this, but I can’t think of a better one.

[Daniel Bates]

(In reply to comment #19)
> (From update of attachment 234284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234284&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:122
> > +template<class T>
> 
> I prefer typename.
> 

Will change before landing.

> > Source/WTF/wtf/StdLibExtras.h:135
> > +    static_assert(std::is_move_constructible<NonConstNonRefQualifiedType>::value || std::is_move_assignable<NonConstNonRefQualifiedType>::value, "T is not movable.");
> 
> I thought we discussed that we want && here, not ||.

Ditto.

(In reply to comment #20)
> (From update of attachment 234284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234284&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:123
> > +ALWAYS_INLINE typename std::remove_reference<T>::type&& move(T&& value)
> 
> To match the model of the rest of WTF, I think we’d want to put a "using WTF::move" at the end of the file. Or maybe we explicitly don’t want that and want to be explicit about using WTF::move at every site where we use it.
> 

I spoke with Anders Carlsson this morning about this (07/03). We prefer being explicit and using the namespace-qualified form at call sites because it makes such usage less error prone (say, in the presence of a "using namespace std" directive) and makes its usages more pronounced in the code.

[Daniel Bates]

(In reply to comment #19)
> (From update of attachment 234284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234284&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:122
> > +template<class T>
> 
> I prefer typename.
> 
> > Source/WTF/wtf/StdLibExtras.h:135
> > +    static_assert(std::is_move_constructible<NonConstNonRefQualifiedType>::value || std::is_move_assignable<NonConstNonRefQualifiedType>::value, "T is not movable.");
> 
> I thought we discussed that we want && here, not ||.

Actually, this compile assert isn't useful using either && or ||. I will remove it before landing the patch. The data type of the passed argument may neither be move constructible nor move assignable (nor both). For instance, PassRef isn't move assignable (i.e. PassRef& operator=(PassRef&&) is neither implicitly nor explicitly defined), but RefPtr has an overloaded constructor and assignment operator that take a PassRef&& (**). Looking at the latter, RefPtr moves the passed PassRef into a local RefPtr using WTF::move(). Then WTF::move() would compile assert that the specified PassRef& isn't movable because PassRef isn't move assignable. But we can move the PassRef& into the RefPtr by (**).

What we really want is to compile assert that the function called with the result of WTF::move() takes an rvalue reference. As far as I know this isn't possible to do from within WTF::move(). :(

[Daniel Bates]

(In reply to comment #22)
[...]
> What we really want is to compile assert that the function called with the result of WTF::move() takes an rvalue reference. As far as I know this isn't possible to do from within WTF::move(). :(

I meant to write:

What we really want is to compile assert that the function called with the result of WTF::move() takes an rvalue reference. As far as I know this isn't possible to do from within WTF::move() in a way that generalizes to all types. We may be able to achieve this effect using template specialization, but this seems brittle.

[Daniel Bates]

Committed r170774: <http://trac.webkit.org/changeset/170774>

[Daniel Bates]

Joseph Pecoraro landed a build fix for the iOS device release build in <http://trac.webkit.org/changeset/170780>.