Bug 176476 – Introduce a utility routine to create a Vector from another based on each individual item

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[youenn fablet]

Currently, we are doing reserveInitialCapacity/iterate-over-each-item/compute-a-value/uncheckedAppend-the-value in a lot of place.
It would be nice to have a method for that pattern.

[youenn fablet]

Created attachment 320059 [details]
Patch

[Darin Adler]

Comment on attachment 320059 [details]
Patch

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

> Source/WebCore/Modules/cache/CacheStorage.cpp:101
> +        startSequentialMatch(m_caches.to<Ref<DOMCache>>().through<copyCacheRef>(), WTFMove(info), WTFMove(options), [this, promise = WTFMove(promise)](ExceptionOr<FetchResponse*>&& result) mutable {

I love the idea, but not the syntax.

Seems strange that callers have to specify the type after to<> here; can’t we deduce that from the return value of the transformer?

The traditional name for this operation is "map", I think. Can we find a way to make m_caches.map<copyCacheRef> work?

[Saam Barati]

(In reply to Darin Adler from comment #2)
> Comment on attachment 320059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320059&action=review
> 
> > Source/WebCore/Modules/cache/CacheStorage.cpp:101
> > +        startSequentialMatch(m_caches.to<Ref<DOMCache>>().through<copyCacheRef>(), WTFMove(info), WTFMove(options), [this, promise = WTFMove(promise)](ExceptionOr<FetchResponse*>&& result) mutable {
> 
> I love the idea, but not the syntax.
> 
> Seems strange that callers have to specify the type after to<> here; can’t
> we deduce that from the return value of the transformer?
> 
> The traditional name for this operation is "map", I think. Can we find a way
> to make m_caches.map<copyCacheRef> work?

I think it'd also be great if we had a lambda variant of this.

[Saam Barati]

FWIW, there was discussion about something similar here:
https://bugs.webkit.org/show_bug.cgi?id=163370

[youenn fablet]

(In reply to Darin Adler from comment #2)
> Comment on attachment 320059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320059&action=review
> 
> > Source/WebCore/Modules/cache/CacheStorage.cpp:101
> > +        startSequentialMatch(m_caches.to<Ref<DOMCache>>().through<copyCacheRef>(), WTFMove(info), WTFMove(options), [this, promise = WTFMove(promise)](ExceptionOr<FetchResponse*>&& result) mutable {
> 
> I love the idea, but not the syntax.
> 
> Seems strange that callers have to specify the type after to<> here; can’t
> we deduce that from the return value of the transformer?
> 
> The traditional name for this operation is "map", I think. Can we find a way
> to make m_caches.map<copyCacheRef> work?

OK, will have another try.

> > The traditional name for this operation is "map", I think. Can we find a way
> > to make m_caches.map<copyCacheRef> work?
> 
> I think it'd also be great if we had a lambda variant of this.

Right, that was the second step.
This one should probably be easier.

[youenn fablet]

(In reply to Saam Barati from comment #4)
> FWIW, there was discussion about something similar here:
> https://bugs.webkit.org/show_bug.cgi?id=163370

Thanks for the pointer!

[youenn fablet]

Created attachment 320143 [details]
map syntax

[youenn fablet]

> > Seems strange that callers have to specify the type after to<> here; can’t
> > we deduce that from the return value of the transformer?
> > 
> > The traditional name for this operation is "map", I think. Can we find a way
> > to make m_caches.map<copyCacheRef> work?
> 
> OK, will have another try.

Uploaded patch is going with m_caches.map<Ref<Cache>, copyCacheRef>.
At this point, I am not sure we can go further and remove the first template parameter.

Auto template parameters in C++17 might probably help to remove the first parameter.
Could probably make it a free function as well.

[youenn fablet]

Comment on attachment 320143 [details]
map syntax

Putting it in r?
Will add unit tests if we decide to go forward with this.

[Saam Barati]

Comment on attachment 320143 [details]
map syntax

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

> Source/WTF/wtf/Vector.h:654
> +    template<typename V, V (*transform)(ValueType&&)> Vector<V> map()

Why can't you figure out V from the return type of the transform using some std:: magic?

[Saam Barati]

Comment on attachment 320143 [details]
map syntax

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

> Source/WebCore/Modules/cache/DOMCache.cpp:412
> +        promise.resolve(result.releaseReturnValue().map<Ref<FetchRequest>, copyRequest>());

Why wouldn't whatever "lambda" version we come up with just handle this? Like, you could just write this as:
map(result.releaseReturnValue(), copyRequest)?
or:
result.releaseReturnValue().map(copyRequest)
depending on if we make it a free function or not

[youenn fablet]

Maybe there is a clever trick I do not know.

Here is my current thinking.
I want the function pointer to be defined as a template value.
For that purpose, I currently need V to be defined.

Since V is defined before transform, and since I want to set transform explicitly, the compiler expects V to be defined explicitly V.

If I was able to define the template with something like template<auto transform>, we would probably figure out the return type with some introspection of transform. That is C++17 IIRC.

If we do not like map<V, transform>(), maybe we can use a struct instead of a function.
We could then have a map function, something like:

struct Mapper {
    static Ref<Cache> transform(const CacheStorageRecord& record) { return record.request.copyRef(); }
};

auto caches = map<Mapper>(records);

(In reply to Saam Barati from comment #10)
> Comment on attachment 320143 [details]
> map syntax
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320143&action=review
> 
> > Source/WTF/wtf/Vector.h:654
> > +    template<typename V, V (*transform)(ValueType&&)> Vector<V> map()
> 
> Why can't you figure out V from the return type of the transform using some
> std:: magic?

[youenn fablet]

(In reply to Saam Barati from comment #11)
> Comment on attachment 320143 [details]
> map syntax
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320143&action=review
> 
> > Source/WebCore/Modules/cache/DOMCache.cpp:412
> > +        promise.resolve(result.releaseReturnValue().map<Ref<FetchRequest>, copyRequest>());
> 
> Why wouldn't whatever "lambda" version we come up with just handle this?
> Like, you could just write this as:
> map(result.releaseReturnValue(), copyRequest)?
> or:
> result.releaseReturnValue().map(copyRequest)
> depending on if we make it a free function or not

A lambda version will always work for those cases but would be slightly less optimal.

[Darin Adler]

Comment on attachment 320143 [details]
map syntax

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

I don’t like the idea of having both a member function named map and a free function named map, but with two different semantics. I am saying review+ but we should straighten this out. I would lean towards having only the free function, and overloading it to have a more efficient version when it’s passed something that can be more readily inlined.

>>> Source/WTF/wtf/Vector.h:654
>>> +    template<typename V, V (*transform)(ValueType&&)> Vector<V> map()
>> 
>> Why can't you figure out V from the return type of the transform using some std:: magic?
> 
> 

std::result_of and std::invoke_result are supposed to solve this problem.

>>> Source/WebCore/Modules/cache/DOMCache.cpp:412
>>> +        promise.resolve(result.releaseReturnValue().map<Ref<FetchRequest>, copyRequest>());
>> 
>> Why wouldn't whatever "lambda" version we come up with just handle this? Like, you could just write this as:
>> map(result.releaseReturnValue(), copyRequest)?
>> or:
>> result.releaseReturnValue().map(copyRequest)
>> depending on if we make it a free function or not
> 
> A lambda version will always work for those cases but would be slightly less optimal.

The compiler never inlines a lambda?

[Darin Adler]

Comment on attachment 320143 [details]
map syntax

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

>>>> Source/WTF/wtf/Vector.h:654
>>>> +    template<typename V, V (*transform)(ValueType&&)> Vector<V> map()
>>> 
>>> Why can't you figure out V from the return type of the transform using some std:: magic?
>> 
>> 
> 
> std::result_of and std::invoke_result are supposed to solve this problem.

std::result_of exists in C++14, does not require C++17

[youenn fablet]

(In reply to Darin Adler from comment #14)
> Comment on attachment 320143 [details]
> map syntax
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320143&action=review
> 
> I don’t like the idea of having both a member function named map and a free
> function named map, but with two different semantics. I am saying review+
> but we should straighten this out. I would lean towards having only the free
> function, and overloading it to have a more efficient version when it’s
> passed something that can be more readily inlined.

Let's not land this version.
The lambda version will be good in most if not all cases.

> The compiler never inlines a lambda?

I was thinking it was not always doing so.
These cases are probably rare and not worth optimizing.
The free function with lambda will be anyway more readable.

[youenn fablet]

Marking at wont fix for now.