[webkit-reviews] review granted: [Bug 227843] [ITP] Improve ResourceLoadStatisticsDatabaseStore's buildList() and use it in more places : [Attachment 433225] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 227843: [ITP] Improve ResourceLoadStatisticsDatabaseStore's buildList() and
use it in more places
https://bugs.webkit.org/show_bug.cgi?id=227843

Attachment 433225: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 433225
  --> https://bugs.webkit.org/attachment.cgi?id=433225
Patch

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

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:330
> +static String buildList(const ContainerType& values)

One day I will add the following to WTF:

template <typename ContainerType, typename SeparatorType, typename
TransformFunctor> String join(const ContainerType& values, const SeparatorType&
separator, TransformFunctor&& functor)
{
    StringBuilder builder;

    bool first = true;
    for (auto& value : values) {
	auto tuple = std::invoke(functor, value);
	if (first) {
	    builder.append(tuple);
	    first = false;
	} else
	    builder.append(separator, tuple);
    }
    return builder.toString();
}

so this becomes

template <typename ContainerType> static String buildList(const ContainerType&
values)
{
    return join(values, ", ", [](auto& value) {
	if constexpr
(std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>)
	    return std::make_tuple('"', value, '"');
	else
	    return value;
    });
}


I'm unclear it it is better :).

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:338
> +	   if constexpr
(std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>) {
> +	       builder.append(builder.isEmpty() ? "" : ", ", value);
> +	   } else {
> +	       builder.append(builder.isEmpty() ? "" : ", ", '"', value, '"');
> +	   }

No need for the braces here. I think it is likely a bit more efficient to use
dedicated bool rather than check isEmpty().