[webkit-reviews] review granted: [Bug 210610] Clean up VectorCocoa createNSArray overloads and add documentation for createNSArray taking a map function : [Attachment 396667] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 210610: Clean up VectorCocoa createNSArray overloads and add documentation
for createNSArray taking a map function
https://bugs.webkit.org/show_bug.cgi?id=210610

Attachment 396667: Patch

https://bugs.webkit.org/attachment.cgi?id=396667&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 396667
  --> https://bugs.webkit.org/attachment.cgi?id=396667
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.