[webkit-reviews] review granted: [Bug 176487] Add a lambda-based map for Vectors : [Attachment 320128] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 176487: Add a lambda-based map for Vectors
https://bugs.webkit.org/show_bug.cgi?id=176487

Attachment 320128: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 320128
  --> https://bugs.webkit.org/attachment.cgi?id=320128
Patch

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

> Source/WTF/wtf/Vector.h:1571
> +	   for (const auto& item : source)

No need for the "const" here.

>> Source/WTF/wtf/Vector.h:1595
>> +}
> 
> I'm slightly hesitant to assume here we're only going to map over Vector
types. Can we come up with a way to do this so we can easily map over HashSet,
HashMap, etc.
> 
> If other folks are interested in mapping over HashSet, HashMap, etc, one
interesting question is, what's the return type? I'd assume map by default
returns its input type, so map over HashSet returns a Vector. But I could
sometimes see mapping a HashSet into a Vector. Maybe we can come up with a nice
default, with the ability to override via templates?

I think the first sentence here is attacking a straw man. This code doesn’t
mean that we are mapping only over Vector types. In C++ we can use overloading
to implement a function for more than one type. But perhaps the question is
"how compatible would this be with other overloads". I don’t think we should
force Youenn to make a more general purpose map function right now, but if
there is some choice he is making here that will make it hard to implement the
others in future, that might be nice.

My design suggestion would be that the WTF::map function produces the same type
it is passed, but then there is map<T> which produces the type T for various
cases where that is easy to support and unambiguous. I think we should approve
this patch without undue worry about that more general map function for the
future; I expect we can implement it later relatively easily or change all
callers if we really have to.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:243
> +	   callback(WTF::map(WTFMove(result.value()), [this](String&& name) {

I believe our coding style asks to put a space between [this] and (String&&
name).

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:656
> +    Vector<int> vector { 1, 1, 1};

I’m not sure we tested all the cases we should.

We tested how it behaves when we pass a Vector, and when we pass a Vector&&,
using WTFMove. But we don’t explicitly test passing a Vector& (which I think
should allow the lambda to modify the values during the mapping process, right,
including allowing them to be explicitly moved, right, or maybe as a matter of
policy we would choose to not allow that?) and passing an actual const Vector&.
Some of these could really be relevant to how we implemented Mapper.

Also, should probably initialize the vector with different values rather than
all with the same value.

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:659
> +    auto mapped = WTF::map(vector, [&](const auto& item) {

Ditto.

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:679
> +    auto mapped = WTF::map(WTFMove(vector), [&](MoveOnly&& item) {

Ditto.