Improve ResourceLoadStatisticsDatabaseStore's buildList() and use it in more places.
Created attachment 433224 [details] Patch
Created attachment 433225 [details] Patch
Comment on attachment 433225 [details] 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().
(In reply to Sam Weinig from comment #3) > Comment on attachment 433225 [details] > 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(). You mean I do not need the curly braces in the else case, right? I thought you'd like the symmetry, since I need them for the `if constexpr` but I'll drop them :) By a dedicated bool, you mean adding a Boolean variable like "isFirst" before the loop then checking it inside the loop and setting it to false? I am personally not convinced it is worse the extra code given that StringBuilder::isEmpty() is implemented like so: ``` bool isEmpty() const { return !m_length; } ```
Created attachment 433250 [details] Patch
Committed r279802 (239566@main): <https://commits.webkit.org/239566@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433250 [details].
<rdar://problem/80406056>
(In reply to Chris Dumez from comment #4) > (In reply to Sam Weinig from comment #3) > > Comment on attachment 433225 [details] > > 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(). > > You mean I do not need the curly braces in the else case, right? I thought > you'd like the symmetry, since I need them for the `if constexpr` but I'll > drop them :) I did mean curly braces. I don't think you need curly braces here though. What kind of error do you get without them? > > By a dedicated bool, you mean adding a Boolean variable like "isFirst" > before the loop then checking it inside the loop and setting it to false? > I am personally not convinced it is worse the extra code given that > StringBuilder::isEmpty() is implemented like so: > ``` > bool isEmpty() const { return !m_length; } > ``` Yeah, you are probably right. Both are probably on the stack so unlikely to be any different.
(In reply to Sam Weinig from comment #8) > (In reply to Chris Dumez from comment #4) > > (In reply to Sam Weinig from comment #3) > > > Comment on attachment 433225 [details] > > > 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(). > > > > You mean I do not need the curly braces in the else case, right? I thought > > you'd like the symmetry, since I need them for the `if constexpr` but I'll > > drop them :) > > I did mean curly braces. I don't think you need curly braces here though. > What kind of error do you get without them? Oh! you're totally right, it builds without the curly brackets. I don't know why I thought `if constexpr` required curly brackets. Anyway, I fixed it in <https://commits.webkit.org/239569@main>, thanks!