Bug 51116 – Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' methods in RefPtr.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jake]

Created attachment 76658 [details]
Patch for potential fix

Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' methods in RefPtr.

Approximately 450 instances of this error are encountered during build:


error C2593: 'operator =' is ambiguous
2>          D:\Qt\WebKit\JavaScriptCore\wtf/RefPtr.h(77): could be 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(std::nullptr_t)'
2>          with
2>          [
2>              T=WebCore::ResourceHandle
2>          ]
2>          D:\Qt\WebKit\JavaScriptCore\wtf/RefPtr.h(74): or       'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(T *)'
2>          with
2>          [
2>              T=WebCore::ResourceHandle
2>          ]
2>          while trying to match the argument list '(WTF::RefPtr<T>, int)'
2>          with
2>          [
2>              T=WebCore::ResourceHandle
2>          ]

To work around this issue, I created a patch (attached) that configures NullPtr.h appropriately for vs2010 (vs2010 has support for nullptr) and replaced occurrences of = 0 with = nullptr.

I believe there are two options for resolving this issue:
1) Use nullptr instead of 0
2) Cast (T*) 0 instead of 0

[Jake]

While my patch only addresses building with vs2010, I propose these changes to NullPtr.h to support backward compatibility with older versions of msvc.


#ifndef NullPtr_h
#define NullPtr_h

// For compilers and standard libraries that do not yet include it, this adds the
// nullptr_t type and nullptr object. They are defined in the same namespaces they
// would be in compiler and library that had the support.

#if !defined(_MSC_VER)
#ifndef __has_feature
    #define __has_feature(feature) 0
#endif

#if !__has_feature(cxx_nullptr)

namespace std {
    class nullptr_t { };
}

extern std::nullptr_t nullptr;

#endif

#elseif  _MSC_VER < 1600

//to maintain compatibility with previous versions of msvc
#ifndef nullptr
#define nullptr 0
#endif

#endif //_MSC_VER

#endif

[Darin Adler]

Comment on attachment 76658 [details]
Patch for potential fix

I’m not sure we should apply this patch, because this leaves us in a state where anyone not using Visual Studio 2010 will introduce breakage any time they use a bare 0.

[Darin Adler]

We can control what overloading does for 0 if we add an operator= that takes an int.

This has the downside that it will allow us to compile if there’s a type mismatch, so it’s probably something we want to do only for Visual Studio 2010 for now.

Another option is to overload for int and make the overload private. That will give us an error on non-Visual-Studio-2010 platforms.

So I suggest something like this:

    public:
    #if HAS_REAL_NULLPTR
        RefPtr& operator=(int value) { ASSERT_UNUSED(value, !value); clear(); return *this; }
    #endif

Then we can add uses of nullptr. Then:

    private:
    #if !HAS_REAL_NULLPTR
        void operator=(int); // never define this
    #endif

[Jake]

Looks simple enough. Have you tried this approach? If not I'll make the change locally and build.

[Alexey Proskuryakov]

Can we just remove RefPtr::operator=(std::nullptr_t)? I don't see a rationale in bug 47756.

[Darin Adler]

*** Bug 51122 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to comment #5)
> Can we just remove RefPtr::operator=(std::nullptr_t)? I don't see a rationale in bug 47756.

Yes, we could.

[Jake]

As an experiment I tried Alexey's suggestion and made the following changes, and webkit built fine with vs2010.

NullPtr.h:

#if !defined(_MSC_VER) || _MSC_VER < 1600
#ifndef __has_feature
    #define __has_feature(feature) 0
#endif

#if !__has_feature(cxx_nullptr)

namespace std {
    class nullptr_t { };
}

extern std::nullptr_t nullptr;

#endif
#endif

RefPtr.h:


#if !defined(_MSC_VER) || _MSC_VER < 1600
        RefPtr& operator=(std::nullptr_t) { clear(); return *this; }
#endif

Much easier than my 450 changes!

[Adam Roben (:aroben)]

(In reply to comment #8)
> As an experiment I tried Alexey's suggestion and made the following changes, and webkit built fine with vs2010.
> 
> NullPtr.h:
> 
> #if !defined(_MSC_VER) || _MSC_VER < 1600
> #ifndef __has_feature
>     #define __has_feature(feature) 0
> #endif
> 
> #if !__has_feature(cxx_nullptr)
> 
> namespace std {
>     class nullptr_t { };
> }
> 
> extern std::nullptr_t nullptr;
> 
> #endif
> #endif
> 
> RefPtr.h:
> 
> 
> #if !defined(_MSC_VER) || _MSC_VER < 1600
>         RefPtr& operator=(std::nullptr_t) { clear(); return *this; }
> #endif
> 
> Much easier than my 450 changes!

Removing the nullptr_t version just for new versions of MSVC seems very fragile. People will add uses of that assignment operator in cross-platform code without realizing they're breaking MSVC.

[Darin Adler]

Alexey’s suggestion was to remove the null_ptr overload from RefPtr entirely, not to remove it just for certain compilers.

[Jake]

Created attachment 77383 [details]
Fixes nullptr_t related build issues on vs2010

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/wtf/NullPtr.h', u'JavaScriptCore/wtf/RefPtr.h', u'WebCore/dom/Document.cpp', u'WebCore/dom/ViewportArguments.h', u'WebCore/page/Geolocation.h']" exit_code: 1
JavaScriptCore/wtf/NullPtr.h:51:  Should have a space between // and comment  [whitespace/comments] [4]
JavaScriptCore/wtf/RefPtr.h:77:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 5 files


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

[Darin Adler]

Comment on attachment 77383 [details]
Fixes nullptr_t related build issues on vs2010

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

Thanks for the patch. Patches need to include change logs. Please submit a new version with a change log.

> JavaScriptCore/wtf/NullPtr.h:36
> +// Don't declare this when building with visual studio 2010+
> +
> +#if !defined(_MSC_VER) || _MSC_VER < 1600

This should go a few lines down:

    #if !__has_feature(cxx_nullptr) && (!defined(_MSC_VER) || _MSC_VER < 1600)

No need to add an extra level of #if.

> JavaScriptCore/wtf/RefPtr.h:77
> +		template<typename U> RefPtr& operator=(const RefPtr<U>&);

Can't land a patch with tabs in it.

> WebCore/dom/ViewportArguments.h:77
> +        , userScalable((bool) ValueAuto)

This fix is incorrect. Bug 50982 has a reviewed patch already in the commit queue to fix this.

> WebCore/page/Geolocation.h:159
> +    class PositionCacheWrapper {
> +    public:
> +        PositionCacheWrapper()
> +        {
> +        }
> +        ~PositionCacheWrapper()
> +        {
> +        }
> +        void setCachedPosition(Geoposition* cachedPosition) {}
> +        Geoposition* cachedPosition() {return 0;}
> +    };

Adding an entire new copy of the class is not the optimal way to fix this. I believe platforms that are not implementing Geolocation should not be compiling this file at all.

This seems unrelated to the compiler and should not be lumped in with the compiler-specific fixes.

[Jake]

Created attachment 77384 [details]
Second attempt at patch to fix nullptr_t related build issues on vs2010

Yikes. I was kicked by the review bot!

[Darin Adler]

Comment on attachment 77384 [details]
Second attempt at patch to fix nullptr_t related build issues on vs2010

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

The rest of my comments from the first patch still apply, except for the one about the tab.

> WebCore/dom/Document.cpp:3175
> -            newFocusedNode = 0;
> +            newFocusedNode = (Node*) 0;

This change looks wrong. Could you tell me more about the warning?

[Jake]

(In reply to comment #15)
> (From update of attachment 77384 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77384&action=review
> 
> The rest of my comments from the first patch still apply, except for the one about the tab.
> 
> > WebCore/dom/Document.cpp:3175
> > -            newFocusedNode = 0;
> > +            newFocusedNode = (Node*) 0;
> 
> This change looks wrong. Could you tell me more about the warning?

Darin you're too fast! I posted the second patch while you were reviewing my first one :)

Regarding the (Node*) 0 issue, for some reason those two lines in Document.cpp failed to compile without the cast.

And sorry about the geolocation thing. It wasn't meant to be included. I'll post a new patch in a couple of minutes.

[Early Warning System Bot]

Attachment 77383 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7238128

[Early Warning System Bot]

Attachment 77384 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7277147

[Jake]

Created attachment 77388 [details]
Latest patch for vs2010 build

Note I put back in to RefPtr the nullptr_t override because without it I got a notice that a build broke.

[Eric Seidel (no email)]

Comment on attachment 77388 [details]
Latest patch for vs2010 build

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

Thanks for the patch!

> WebCore/dom/Document.cpp:3175
> +            newFocusedNode = (Node*) 0;

We don't use c-style casts.  Why is this code modifying the PassRefPTr anyway?  We can call .clear() instead, I think?

> WebCore/dom/ViewportArguments.h:77
> +        , userScalable((bool) ValueAuto)

Again, we don't use c-style casts.  We should either disable the failing warning for the file, or we should use static_cast or find some better way to avoid the error.

[Jake]

(In reply to comment #20)
> (From update of attachment 77388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77388&action=review
> 
> Thanks for the patch!
> 
> > WebCore/dom/Document.cpp:3175
> > +            newFocusedNode = (Node*) 0;
> 
> We don't use c-style casts.  Why is this code modifying the PassRefPTr anyway?  We can call .clear() instead, I think?
> 
> > WebCore/dom/ViewportArguments.h:77
> > +        , userScalable((bool) ValueAuto)
> 
> Again, we don't use c-style casts.  We should either disable the failing warning for the file, or we should use static_cast or find some better way to avoid the error.

I have no idea whether .clear() can be called - I didn't write the code. All I know is newFocusedNode = 0; doesn't compile.

And secondly, userScalable(ValueAuto) is far worse than the cast - at least with the cast its clear what is going on - userScalable is a bool and ValueAuto isn't.

[Jake]

Created attachment 77429 [details]
Latest patch for vs2010 build

This patch uses static_cast instead of c style casts.

[Jake]

Created attachment 77433 [details]
Fixes nullptr_t related build issues on vs2010

Removed from all of the WTF smart pointer classes the operator =(nullptr_t) override so they'll build with visual studio 2010.

[Eric Seidel (no email)]

nullptr_t was all added in:
http://trac.webkit.org/changeset/69970

[Eric Seidel (no email)]

This seems sane to me, but I think it would be best for Darin or Anders to comment here.  Thanks again for your work on this patch!

[Darin Adler]

Comment on attachment 77433 [details]
Fixes nullptr_t related build issues on vs2010

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

Thanks for persisting on this. I really appreciate you taking the time to do this.

> JavaScriptCore/wtf/NullPtr.h:38
> -#if !__has_feature(cxx_nullptr)
> +#if !__has_feature(cxx_nullptr) && (!defined(_MSC_VER) || _MSC_VER < 1600)

This change is fine.

> JavaScriptCore/wtf/OwnArrayPtr.h:79
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>      OwnArrayPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

This change is wrong in two ways:

    1) The #if in these files should not be specifically _MSC_VER, but rather an #if about having a real nullptr and nullptr_t rather a fake one. That define for that should be set up in NullPtr.h. We want this to be true for newer versions of MSVC newer versions of the clang compiler. One way to do this that fits in well with how we do ifdef's in WebKit would be to define HAVE_NULLPTR in NullPtr.h only if a real nullptr is present. Then we could use #if !HAVE(NULLPTR) at these call sites.

    2) We need the nullptr overload even for real nullptr compilers when LOOSE_OWN_ARRAY_PTR is not defined, so the expression has to be written taking that into account.

> JavaScriptCore/wtf/OwnPtr.h:78
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>          OwnPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

Same two issues as OwnArrayPtr.h.

> JavaScriptCore/wtf/PassOwnArrayPtr.h:78
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>      PassOwnArrayPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

Same two issues as OwnArrayPtr.h.

> JavaScriptCore/wtf/PassOwnPtr.h:77
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>          PassOwnPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

Same two issues as OwnArrayPtr.h.

> JavaScriptCore/wtf/PassRefPtr.h:96
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>          PassRefPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

Issue (1) above applies here too. Or we could remove the overload for nullptr_t entirely.

> JavaScriptCore/wtf/RefPtr.h:79
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>          RefPtr& operator=(std::nullptr_t) { clear(); return *this; }
> +#endif

Same as PassRefPtr.h.

> JavaScriptCore/wtf/RetainPtr.h:92
> +#if !defined(_MSC_VER) || _MSC_VER < 1600
>          RetainPtr& operator=(std::nullptr_t) { clear(); return *this; }
> -
> +#endif

Same as PassRefPtr.h.

[Jake]

Ah yes HAVE_NULLPTR makes more sense.

How about this?

In NullPtr.h:


#if defined(LOOSE_OWN_ARRAY_PTR) || (!__has_feature(cxx_nullptr) && (!defined(_MSC_VER) || _MSC_VER < 1600))

namespace std {
    class nullptr_t { };
}

extern std::nullptr_t nullptr;
#else
#define HAVE_NULLPTR 1
#endif


and then surrounding the operator overrides:

#if !HAVE(NULLPTR)
oper...
#endif

I'm assuming HAVE() is defined somewhere?

[Darin Adler]

Committed r74695: <http://trac.webkit.org/changeset/74695>

[Darin Adler]

Jake, I took the liberty of reworking your patch and landing it.

Since I didn't actually compile with Visual Studio 2010, there may still be some ambiguous operator= problems, or other problems. I suggest new separate bug reports for individual remaining problems. Please feel free to cc me on any of those bugs.

[Eric Seidel (no email)]

It's a little odd to have a HAVE definition outside of Platform.h no?

I guess any code which wants to actually use nullptr_t will have to include NullPtr.h anyway.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74695 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
media/adopt-node-crash.html

[Darin Adler]

(In reply to comment #30)
> It's a little odd to have a HAVE definition outside of Platform.h no?

Sure, it would be OK to move that logic to Platform.h.

On the one hand, I am not sure that it's good to continue making that an everything file, but it would be good to put language and compiler things all in one place, although I don’t think there’s a good clear purpose to that file at the moment. It doesn't make sense to me that the platform independence macros are in the same file as their definitions for all platforms. I think Maciej designed a new way to do it.

Anyway, moving would be fine with me.

Other files defining HAVE outside Platform.h include Assertions.h, RenderObject.h and GraphicsContextCG.cpp.

> I guess any code which wants to actually use nullptr_t will have to include NullPtr.h anyway.

Yes, that's what I was thinking.

[Jake]

Thanks Darin! I'll get latest and do a new build.

-Jake

(In reply to comment #32)
> (In reply to comment #30)
> > It's a little odd to have a HAVE definition outside of Platform.h no?
> 
> Sure, it would be OK to move that logic to Platform.h.
> 
> On the one hand, I am not sure that it's good to continue making that an everything file, but it would be good to put language and compiler things all in one place, although I don’t think there’s a good clear purpose to that file at the moment. It doesn't make sense to me that the platform independence macros are in the same file as their definitions for all platforms. I think Maciej designed a new way to do it.
> 
> Anyway, moving would be fine with me.
> 
> Other files defining HAVE outside Platform.h include Assertions.h, RenderObject.h and GraphicsContextCG.cpp.
> 
> > I guess any code which wants to actually use nullptr_t will have to include NullPtr.h anyway.
> 
> Yes, that's what I was thinking.

(In reply to comment #29)
> Jake, I took the liberty of reworking your patch and landing it.
> 
> Since I didn't actually compile with Visual Studio 2010, there may still be some ambiguous operator= problems, or other problems. I suggest new separate bug reports for individual remaining problems. Please feel free to cc me on any of those bugs.

[Eric Seidel (no email)]

(In reply to comment #32)
> (In reply to comment #30)
> > It's a little odd to have a HAVE definition outside of Platform.h no?
> 
> Sure, it would be OK to move that logic to Platform.h.
> 
> On the one hand, I am not sure that it's good to continue making that an everything file, but it would be good to put language and compiler things all in one place, although I don’t think there’s a good clear purpose to that file at the moment. It doesn't make sense to me that the platform independence macros are in the same file as their definitions for all platforms.

Yeah.  It's unfortunate that Platform.h has all of these, however the way the current HAVE() macro works, if a HAVE_ is not defined then it's assumed false:
#define HAVE(WTF_FEATURE) (defined HAVE_##WTF_FEATURE  && HAVE_##WTF_FEATURE)

Which could lead to folks compiling things in ways they didn't intend if all the HAVE_ definitions aren't in one place.

> I think Maciej designed a new way to do it.

Unfortunately he's been saying that for years. :)  I'm looking forward to it though!

[Eric Seidel (no email)]

(In reply to comment #34)
> > I think Maciej designed a new way to do it.
> 
> Unfortunately he's been saying that for years. :)  I'm looking forward to it though!

Perhaps too sharp a jab.  I do look forward to his re-write and I hope we see it in a patch soon.

Just as I'm sure you look forward to the numerous blog-posts and LayoutTest re-orgs I've promised you over the years... :(