Bug 123707 – [Cocoa] Wrappers' -copyWithZone: should copy if the collection is mutable

• Component: WebKit2
• Status: RESOLVED FIXED

[Cocoa] Wrappers' -copyWithZone: should copy if the collection is mutable

Created attachment 215870 [details]
If the collection is mutable, make a copy

[Geoffrey Garen]

Comment on attachment 215870 [details]
If the collection is mutable, make a copy

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

> Source/WebKit2/Shared/ImmutableArray.h:68
> +    const Vector<RefPtr<APIObject>> entries() { return m_entries; }

Should this be Vector& instead of Vector? As written, each call to entries() will make a copy.

> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63
> +    auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries();

If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake).

Comment on attachment 215870 [details]
If the collection is mutable, make a copy

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

>> Source/WebKit2/Shared/ImmutableArray.h:68
>> +    const Vector<RefPtr<APIObject>> entries() { return m_entries; }
> 
> Should this be Vector& instead of Vector? As written, each call to entries() will make a copy.

Yes.

>> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63
>> +    auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries();
> 
> If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake).

This needs to be a copy, not a reference.

Created attachment 215872 [details]
If the collection is mutable, make a copy

[Geoffrey Garen]

Comment on attachment 215872 [details]
If the collection is mutable, make a copy

r=me

[Geoffrey Garen]

> >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63
> >> +    auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries();
> > 
> > If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake).
> 
> This needs to be a copy, not a reference.

What I meant was that, if you had used

const ImmutableArray::VectorType& entries = ...

you would have gotten an error, due to taking a reference to a temporary, which might have alerted you to the accidental copy caused by the return type of entries().

I think about stuff like this because I'm still not sure when we should use auto, and when we should not.

(In reply to comment #6)
> > >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63
> > >> +    auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries();
> > > 
> > > If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake).
> > 
> > This needs to be a copy, not a reference.
> 
> What I meant was that, if you had used
> 
> const ImmutableArray::VectorType& entries = ...
> 
> you would have gotten an error, due to taking a reference to a temporary, which might have alerted you to the accidental copy caused by the return type of entries().

But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference.

Committed <http://trac.webkit.org/r158538>.

[Geoffrey Garen]

> But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference.

Uh oh. Since the patch you committed passes a reference from _array (and _dictionary) to adopt(), and adopt() calls swap() -- does this mean that -copyWithZone will remove all entries from self?

I am demonstrating my limitations as a reviewer :(.

(In reply to comment #9)
> > But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference.
> 
> Uh oh. Since the patch you committed passes a reference from _array (and _dictionary) to adopt(), and adopt() calls swap() -- does this mean that -copyWithZone will remove all entries from self?

The locals’ types aren’t const references. They are a Vector and a MapType.

(In reply to comment #8)
> Committed <http://trac.webkit.org/r158538>.

I shouldn’t have landed this patch without a WebKit2 owner’s permission. I reverted the change in <http://trac.webkit.org/r158552>.

…and committed <http://trac.webkit.org/r158553>.