[webkit-reviews] review granted: [Bug 74159] Fix HashMap<..., OwnPtr<...> >::add compilation errors : [Attachment 118521] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adrienne Walker <enne at google.com>'s
request for review:
Bug 74159: Fix HashMap<..., OwnPtr<...> >::add compilation errors
https://bugs.webkit.org/show_bug.cgi?id=74159

Attachment 118521: Patch
https://bugs.webkit.org/attachment.cgi?id=118521&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118521&action=review


r=me, but please land my revised version

>> Source/JavaScriptCore/wtf/OwnPtr.h:44
>> +	    explicit OwnPtr(const std::nullptr_t&) : m_ptr(0) { }
> 
> I see no reason for this to be explicit.
> 
> This might work for some compilers, but with others I fear it may break the
build. The EWS seems to like it OK.
> 
> Since you did not include the entire error, I cannot be sure, but I suspect
there may be a way to resolve this entirely in HashTraits.h. For example, it
may be a simple matter of overloading some function to take a nullptr_t.
> 
> I am not going to say review+ yet because of the “explicit” issue and the
fact that I’d like to see the rest of the error.

I changed my mind about all the rest of my comment. This is a good change.

But we should not use “explicit” here. It’s good and helpful to allow implicit
conversion from nullptr_t to OwnPtr.

It should read:

    OwnPtr(std::nullptr_t) : m_ptr(0) { }

There is no reason to use const std::nullptr_t& instead of just std::nullptr_t.
See PassOwnPtr, which already has this constructor.