Clean up VectorCocoa createNSArray overloads and add documentation for createNSArray taking a map function.
Created attachment 396661 [details] Patch
Created attachment 396662 [details] Patch
Created attachment 396665 [details] Patch
Comment on attachment 396665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396665&action=review > Source/WTF/ChangeLog:8 > + Remove unncessary local variable. Rename "map" type in createNSArray function for clarity unncessary => unnecessary
Created attachment 396667 [details] Patch
Comment on attachment 396667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396667&action=review > Source/WTF/wtf/cocoa/VectorCocoa.h:56 > +// An overload that takes a map function and returns a NSArray whose elements are the > +// result of calling the map function on each vector element. Like the makeNSArrayElement > +// function, the map function can return either a RetainPtr<id> or an id if the value > +// is autoreleased. When the map function is makeNSArrayElement then this function > +// behaves identically to createNSArray function that does not take a map function. I’m not thrilled with this comment. Here’s my cut: // This overload of createNSArray takes a function to map each vector element to an Objective-C object. // The map function has the same interface as the makeVectorElement function above, but can be any // function including a lambda, a function-like object, or Function<>. I’m sure you can do even better. > Source/WTF/wtf/cocoa/VectorCocoa.h:83 > - constexpr const VectorElementType* typedNull = nullptr; > + constexpr VectorElementType* typedNull = nullptr; Please don’t make this change. The const there is about the type of the pointed-to item. This const should not be removed. In some coding styles it would be written as "constexpr VectorElementType const *", in case that makes it clearer to you. If this was not a pointer type, then the change would make sense to me, removing a redundant const, but that’s based on a misunderstanding here.
Comment on attachment 396667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396667&action=review >> Source/WTF/wtf/cocoa/VectorCocoa.h:56 >> +// behaves identically to createNSArray function that does not take a map function. > > I’m not thrilled with this comment. Here’s my cut: > > // This overload of createNSArray takes a function to map each vector element to an Objective-C object. > // The map function has the same interface as the makeVectorElement function above, but can be any > // function including a lambda, a function-like object, or Function<>. > > I’m sure you can do even better. You're tempting me...I'll just take your cut. >> Source/WTF/wtf/cocoa/VectorCocoa.h:83 >> + constexpr VectorElementType* typedNull = nullptr; > > Please don’t make this change. The const there is about the type of the pointed-to item. This const should not be removed. In some coding styles it would be written as "constexpr VectorElementType const *", in case that makes it clearer to you. If this was not a pointer type, then the change would make sense to me, removing a redundant const, but that’s based on a misunderstanding here. You're right, I'm wrong. I misread the code.
Thanks for the review, btw
Comment on attachment 396667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396667&action=review >>> Source/WTF/wtf/cocoa/VectorCocoa.h:56 >>> +// behaves identically to createNSArray function that does not take a map function. >> >> I’m not thrilled with this comment. Here’s my cut: >> >> // This overload of createNSArray takes a function to map each vector element to an Objective-C object. >> // The map function has the same interface as the makeVectorElement function above, but can be any >> // function including a lambda, a function-like object, or Function<>. >> >> I’m sure you can do even better. > > You're tempting me...I'll just take your cut. The same interface as makeVectorElement? The same interface as makeNSArrayElement
Created attachment 396675 [details] To Land
Comment on attachment 396675 [details] To Land Clearing flags on attachment: 396675 Committed r260202: <https://trac.webkit.org/changeset/260202>
All reviewed patches have been landed. Closing bug.
<rdar://problem/61895756>