[Cocoa] Wrappers' -copyWithZone: should copy if the collection is mutable
Created attachment 215870 [details] If the collection is mutable, make a copy
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
Comment on attachment 215872 [details] If the collection is mutable, make a copy r=me
> >> 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>.
> 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>.