[webkit-reviews] review denied: [Bug 51116] Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' methods in RefPtr. : [Attachment 77433] Fixes nullptr_t related build issues on vs2010

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jake <jake at jakeonthenet.com>'s request
for review:
Bug 51116: Building webkit with Visual Studio 2010 fails due to ambiguous
'operator =' methods in RefPtr.
https://bugs.webkit.org/show_bug.cgi?id=51116

Attachment 77433: Fixes nullptr_t related build issues on vs2010
https://bugs.webkit.org/attachment.cgi?id=77433&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.