Allow Variants of RetainPtrs
Created attachment 307313 [details] Patch
What was the error? Could it be fixed by having Variant use std::addressof, which is designed to work around objects that overload operator&?
../Source/WTF/wtf/Variant.h:1798:12: error: non-const lvalue reference to type 'typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'WTF::RetainPtr<const __CFData *>') cannot bind to a value of unrelated type 'PtrType' (aka 'const __CFData *') return *( ^~ ../Source/WTF/wtf/Variant.h:1768:12: note: in instantiation of function template specialization 'WTF::get<1, WTF::RefPtr<TestWebKitAPI::RefLogger>, WTF::RetainPtr<const __CFData *> >' requested here return get<__type_index<_Type,_Types...>::__value>(__v); ^ ../Source/WTF/wtf/Variant.h:1868:26: note: in instantiation of function template specialization 'WTF::get<WTF::RetainPtr<const __CFData *>, WTF::RefPtr<TestWebKitAPI::RefLogger>, WTF::RetainPtr<const __CFData *> >' requested here return __visitor(get<_Type>(__v)); ^ ../Source/WTF/wtf/Variant.h:1876:10: note: in instantiation of function template specialization 'WTF::__visitor_table<WTF::Visitor<(lambda at ../Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:124:9), (lambda at ../Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:125:9)> &, WTF::RefPtr<TestWebKitAPI::RefLogger>, WTF::RetainPtr<const __CFData *> >::__trampoline_func<WTF::RetainPtr<const __CFData *> >' requested here &__trampoline_func<_Types>... ^ ../Source/WTF/wtf/Variant.h:1884:48: note: in instantiation of static data member 'WTF::__visitor_table<WTF::Visitor<(lambda at ../Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:124:9), (lambda at ../Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:125:9)> &, WTF::RefPtr<TestWebKitAPI::RefLogger>, WTF::RetainPtr<const __CFData *> >::__trampoline' requested here : __visitor_table<_Visitor,_Types...>::__trampoline[__v.index()](__visitor,__v); ^ ../Source/WTF/wtf/Variant.h:1606:31: error: no viable overloaded '=' get<_Index>(*this)=std::forward<_Type>(__x); ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ ../Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:134:13: note: in instantiation of function template specialization 'WTF::Variant<WTF::RefPtr<TestWebKitAPI::RefLogger>, WTF::RetainPtr<const __CFData *> >::operator=<WTF::RetainPtr<const __CFData *> >' requested here variant = WTFMove(retainPtr); ^ ../Source/WTF/wtf/RetainPtr.h:107:16: note: candidate function not viable: 'this' argument has type 'const typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'const WTF::RetainPtr<const __CFData *>'), but method is not marked const RetainPtr& operator=(const RetainPtr&); ^ ../Source/WTF/wtf/RetainPtr.h:108:37: note: candidate function not viable: 'this' argument has type 'const typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'const WTF::RetainPtr<const __CFData *>'), but method is not marked const template<typename U> RetainPtr& operator=(const RetainPtr<U>&); ^ ../Source/WTF/wtf/RetainPtr.h:109:16: note: candidate function not viable: 'this' argument has type 'const typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'const WTF::RetainPtr<const __CFData *>'), but method is not marked const RetainPtr& operator=(PtrType); ^ ../Source/WTF/wtf/RetainPtr.h:112:16: note: candidate function not viable: 'this' argument has type 'const typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'const WTF::RetainPtr<const __CFData *>'), but method is not marked const RetainPtr& operator=(RetainPtr&&); ^ ../Source/WTF/wtf/RetainPtr.h:113:37: note: candidate function not viable: 'this' argument has type 'const typename __indexed_type<1L, RefPtr<RefLogger>, RetainPtr<const __CFData *> >::__type' (aka 'const WTF::RetainPtr<const __CFData *>'), but method is not marked const template<typename U> RetainPtr& operator=(RetainPtr<U>&&); ^ ../Source/WTF/wtf/RetainPtr.h:110:37: note: candidate template ignored: could not match 'U *' against 'WTF::RetainPtr<const __CFData *>' template<typename U> RetainPtr& operator=(U*); ^ 2 errors generated.
I tried using std::addressof instead of & in WTF::Variant, but it still failed for some reason. Maybe we should fix that and add a test with a class that has an operator& that returns a different type, but this fixes my problem and I think it improves our code.
(In reply to Alex Christensen from comment #4) > I tried using std::addressof instead of & in WTF::Variant, but it still > failed for some reason. Maybe we should fix that and add a test with a > class that has an operator& that returns a different type, but this fixes my > problem and I think it improves our code. By "improves our code" do you mean it is good to get rid of the operator& for RetainPtr? It seemed like an ok use of operator& to me.
Created attachment 307332 [details] Fix for Variant Here is a fix for Variant, and a small test case. The test could definitely be fleshed out, and maybe an explicit use of RetainPtr<> could be added if you end up keeping the RetainPtr operator&.
Attachment 307332 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Variant.h:1791: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1792: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1792: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WTF/wtf/Variant.h:1800: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1801: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1801: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WTF/wtf/Variant.h:1821: Missing spaces around != [whitespace/operators] [3] ERROR: Source/WTF/wtf/Variant.h:1821: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1826: Missing spaces around != [whitespace/operators] [3] ERROR: Source/WTF/wtf/Variant.h:1826: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1832: Missing space after , [whitespace/comma] [3] ERROR: Source/WTF/wtf/Variant.h:1839: Missing space after , [whitespace/comma] [3] ERROR: Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:233: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:241: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Variant.cpp:244: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] Total errors found: 15 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Aha! I had missed the &get needing to be changed to std::addressof(get when I tried it. Thanks Sam! I think removing operator& from RetainPtr is an improvement because using it requires you to know that it's only ok to use with nullptr, and there are only runtime checks as it was. I think replacing & with std::addressof in our Variant implementation is an improvement because it will work if we make variants in the future of types that have overloaded operator& that return a different type. I'm going to commit both.
http://trac.webkit.org/r215450