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.
Created attachment 320059 [details] Patch
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?
(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.
FWIW, there was discussion about something similar here: https://bugs.webkit.org/show_bug.cgi?id=163370
(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.
(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!
Created attachment 320143 [details] map 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. 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.
Comment on attachment 320143 [details] map syntax Putting it in r? Will add unit tests if we decide to go forward with this.
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?
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
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?
(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.
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?
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
(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.
Marking at wont fix for now.