Title: [128203] trunk/Source/WTF
Revision
128203
Author
[email protected]
Date
2012-09-11 10:39:08 -0700 (Tue, 11 Sep 2012)

Log Message

Clang doesn't optimize away undefined OwnPtr copy constructor
https://bugs.webkit.org/show_bug.cgi?id=74625

Reviewed by Anders Carlsson.

Original patch by Anders Carlsson, with a minor edit.

The publicly declared-but-not-defined copy constructor is a compiler
optimization-dependent landmine. Clang often fails to optimize the use
of this function out, leading to internal linkage errors for the missing
definition. gcc doesn't have this problem and optimizes that function
out, leading to code that inconsistently fails to link across platforms.

As a partial fix for this problem, on any compiler that supports C++11
move semantics, replace the bogus copy constructor with the move
constructors.  In the future, if all compilers support this, then the
copy constructor can be removed.

This still leaves other compilers that don't support move semantics
like Visual Studio vulnerable to linking inconsistencies.

* wtf/OwnPtr.h:
(OwnPtr):
(WTF):
(WTF::::OwnPtr):
(WTF::=):

Modified Paths

Diff

Modified: trunk/Source/WTF/ChangeLog (128202 => 128203)


--- trunk/Source/WTF/ChangeLog	2012-09-11 17:29:54 UTC (rev 128202)
+++ trunk/Source/WTF/ChangeLog	2012-09-11 17:39:08 UTC (rev 128203)
@@ -1,3 +1,32 @@
+2012-09-11  Adrienne Walker  <[email protected]>
+
+        Clang doesn't optimize away undefined OwnPtr copy constructor
+        https://bugs.webkit.org/show_bug.cgi?id=74625
+
+        Reviewed by Anders Carlsson.
+
+        Original patch by Anders Carlsson, with a minor edit.
+
+        The publicly declared-but-not-defined copy constructor is a compiler
+        optimization-dependent landmine. Clang often fails to optimize the use
+        of this function out, leading to internal linkage errors for the missing
+        definition. gcc doesn't have this problem and optimizes that function
+        out, leading to code that inconsistently fails to link across platforms.
+
+        As a partial fix for this problem, on any compiler that supports C++11
+        move semantics, replace the bogus copy constructor with the move
+        constructors.  In the future, if all compilers support this, then the
+        copy constructor can be removed.
+
+        This still leaves other compilers that don't support move semantics
+        like Visual Studio vulnerable to linking inconsistencies.
+
+        * wtf/OwnPtr.h:
+        (OwnPtr):
+        (WTF):
+        (WTF::::OwnPtr):
+        (WTF::=):
+
 2012-09-11  Raphael Kubo da Costa  <[email protected]>
 
         [EFL] Rewrite the EFL-related Find modules

Modified: trunk/Source/WTF/wtf/OwnPtr.h (128202 => 128203)


--- trunk/Source/WTF/wtf/OwnPtr.h	2012-09-11 17:29:54 UTC (rev 128202)
+++ trunk/Source/WTF/wtf/OwnPtr.h	2012-09-11 17:39:08 UTC (rev 128203)
@@ -22,6 +22,7 @@
 #define WTF_OwnPtr_h
 
 #include <wtf/Assertions.h>
+#include <wtf/Noncopyable.h>
 #include <wtf/NullPtr.h>
 #include <wtf/OwnPtrCommon.h>
 #include <wtf/TypeTraits.h>
@@ -36,6 +37,11 @@
     template<typename T> PassOwnPtr<T> adoptPtr(T*);
 
     template<typename T> class OwnPtr {
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        // If rvalue references are not supported, the copy constructor is
+        // public so OwnPtr cannot be marked noncopyable. See note below.
+        WTF_MAKE_NONCOPYABLE(OwnPtr);
+#endif
     public:
         typedef typename RemovePointer<T>::Type ValueType;
         typedef ValueType* PtrType;
@@ -46,11 +52,13 @@
         // See comment in PassOwnPtr.h for why this takes a const reference.
         template<typename U> OwnPtr(const PassOwnPtr<U>& o);
 
+#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
         // This copy constructor is used implicitly by gcc when it generates
         // transients for assigning a PassOwnPtr<T> object to a stack-allocated
         // OwnPtr<T> object. It should never be called explicitly and gcc
         // should optimize away the constructor when generating code.
         OwnPtr(const OwnPtr<ValueType>&);
+#endif
 
         ~OwnPtr() { deleteOwnedPtr(m_ptr); }
 
@@ -73,10 +81,21 @@
         OwnPtr& operator=(std::nullptr_t) { clear(); return *this; }
         template<typename U> OwnPtr& operator=(const PassOwnPtr<U>&);
 
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        OwnPtr(OwnPtr&&);
+        template<typename U> OwnPtr(OwnPtr<U>&&);
+
+        OwnPtr& operator=(OwnPtr&&);
+        template<typename U> OwnPtr& operator=(OwnPtr<U>&&);
+#endif
+
         void swap(OwnPtr& o) { std::swap(m_ptr, o.m_ptr); }
 
     private:
-        OwnPtr& operator=(const OwnPtr<T>&);
+#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        // If rvalue references are supported, noncopyable takes care of this.
+        OwnPtr& operator=(const OwnPtr&);
+#endif
 
         // We should never have two OwnPtrs for the same underlying object (otherwise we'll get
         // double-destruction), so these equality operators should never be needed.
@@ -132,6 +151,38 @@
         return *this;
     }
 
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+    template<typename T> inline OwnPtr<T>::OwnPtr(OwnPtr<T>&& o)
+        : m_ptr(o.leakPtr())
+    {
+    }
+
+    template<typename T> template<typename U> inline OwnPtr<T>::OwnPtr(OwnPtr<U>&& o)
+        : m_ptr(o.leakPtr())
+    {
+    }
+
+    template<typename T> inline OwnPtr<T>& OwnPtr<T>::operator=(OwnPtr<T>&& o)
+    {
+        PtrType ptr = m_ptr;
+        m_ptr = o.leakPtr();
+        ASSERT(!ptr || m_ptr != ptr);
+        deleteOwnedPtr(ptr);
+
+        return *this;
+    }
+
+    template<typename T> template<typename U> inline OwnPtr<T>& OwnPtr<T>::operator=(OwnPtr<U>&& o)
+    {
+        PtrType ptr = m_ptr;
+        m_ptr = o.leakPtr();
+        ASSERT(!ptr || m_ptr != ptr);
+        deleteOwnedPtr(ptr);
+
+        return *this;
+    }
+#endif
+
     template<typename T> inline void swap(OwnPtr<T>& a, OwnPtr<T>& b)
     {
         a.swap(b);
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to