Bug 51116 - Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' methods in RefPtr.
Summary: Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' m...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 51122 (view as bug list)
Depends on:
Blocks: 30718
  Show dependency treegraph
 
Reported: 2010-12-15 09:55 PST by Jake
Modified: 2011-01-03 06:32 PST (History)
13 users (show)

See Also:


Attachments
Patch for potential fix (96.83 KB, patch)
2010-12-15 09:55 PST, Jake
no flags Details | Formatted Diff | Diff
Fixes nullptr_t related build issues on vs2010 (3.42 KB, patch)
2010-12-23 16:33 PST, Jake
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Second attempt at patch to fix nullptr_t related build issues on vs2010 (3.36 KB, patch)
2010-12-23 16:41 PST, Jake
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Latest patch for vs2010 build (3.38 KB, patch)
2010-12-23 17:49 PST, Jake
eric: review-
Details | Formatted Diff | Diff
Latest patch for vs2010 build (2.93 KB, patch)
2010-12-24 14:39 PST, Jake
no flags Details | Formatted Diff | Diff
Fixes nullptr_t related build issues on vs2010 (6.92 KB, patch)
2010-12-24 17:45 PST, Jake
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake 2010-12-15 09:55:17 PST
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
Comment 1 Jake 2010-12-15 10:02:06 PST
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
Comment 2 Darin Adler 2010-12-15 11:16:05 PST
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.
Comment 3 Darin Adler 2010-12-15 11:19:25 PST
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
Comment 4 Jake 2010-12-15 11:24:21 PST
Looks simple enough. Have you tried this approach? If not I'll make the change locally and build.
Comment 5 Alexey Proskuryakov 2010-12-15 15:10:48 PST
Can we just remove RefPtr::operator=(std::nullptr_t)? I don't see a rationale in bug 47756.
Comment 6 Darin Adler 2010-12-15 18:53:05 PST
*** Bug 51122 has been marked as a duplicate of this bug. ***
Comment 7 Darin Adler 2010-12-15 18:53:26 PST
(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.
Comment 8 Jake 2010-12-17 09:36:21 PST
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!
Comment 9 Adam Roben (:aroben) 2010-12-17 09:43:28 PST
(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.
Comment 10 Darin Adler 2010-12-17 10:17:33 PST
Alexey’s suggestion was to remove the null_ptr overload from RefPtr entirely, not to remove it just for certain compilers.
Comment 11 Jake 2010-12-23 16:33:57 PST
Created attachment 77383 [details]
Fixes nullptr_t related build issues on vs2010
Comment 12 WebKit Review Bot 2010-12-23 16:35:23 PST
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.
Comment 13 Darin Adler 2010-12-23 16:39:36 PST
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.
Comment 14 Jake 2010-12-23 16:41:29 PST
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!
Comment 15 Darin Adler 2010-12-23 16:43:51 PST
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?
Comment 16 Jake 2010-12-23 16:50:11 PST
(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.
Comment 17 Early Warning System Bot 2010-12-23 16:58:08 PST
Attachment 77383 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7238128
Comment 18 Early Warning System Bot 2010-12-23 17:20:26 PST
Attachment 77384 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7277147
Comment 19 Jake 2010-12-23 17:49:32 PST
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.
Comment 20 Eric Seidel (no email) 2010-12-24 08:58:57 PST
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.
Comment 21 Jake 2010-12-24 14:21:18 PST
(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.
Comment 22 Jake 2010-12-24 14:39:44 PST
Created attachment 77429 [details]
Latest patch for vs2010 build

This patch uses static_cast instead of c style casts.
Comment 23 Jake 2010-12-24 17:45:18 PST
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.
Comment 24 Eric Seidel (no email) 2010-12-24 19:01:19 PST
nullptr_t was all added in:
http://trac.webkit.org/changeset/69970
Comment 25 Eric Seidel (no email) 2010-12-24 19:01:52 PST
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!
Comment 26 Darin Adler 2010-12-26 13:19:27 PST
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.
Comment 27 Jake 2010-12-27 11:55:12 PST
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?
Comment 28 Darin Adler 2010-12-27 13:42:14 PST
Committed r74695: <http://trac.webkit.org/changeset/74695>
Comment 29 Darin Adler 2010-12-27 13:44:16 PST
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.
Comment 30 Eric Seidel (no email) 2010-12-27 14:10:11 PST
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.
Comment 31 WebKit Review Bot 2010-12-27 15:02:46 PST
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
Comment 32 Darin Adler 2010-12-27 16:14:20 PST
(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.
Comment 33 Jake 2010-12-27 16:16:57 PST
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.
Comment 34 Eric Seidel (no email) 2010-12-27 17:35:05 PST
(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!
Comment 35 Eric Seidel (no email) 2010-12-27 17:39:15 PST
(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... :(