Bug 170923 – Allow Variants of RetainPtrs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Allow Variants of RetainPtrs

[Alex Christensen]

Created attachment 307313 [details]
Patch

[Sam Weinig]

What was the error? Could it be fixed by having Variant use std::addressof, which is designed to work around objects that overload operator&?

[Alex Christensen]

../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.

[Alex Christensen]

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.

[Sam Weinig]

(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.

[Sam Weinig]

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&.

[Build Bot]

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.

[Alex Christensen]

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.

[Alex Christensen]

http://trac.webkit.org/r215450