Bug 213708 – Use more efficient makeString() instead of StringBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Alexey Shvayka]

Use more efficient makeString() instead of StringBuilder

[Alexey Shvayka]

Created attachment 403024 [details]
Patch

[Alexey Shvayka]

Comment on attachment 403024 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:170
> +template<typename... StringTypes> static String makeArgumentTypeErrorMessage(unsigned argumentIndex, const char* argumentName, const char* interfaceName, const char* functionName, StringTypes ...strings)

I think it's nice to keep this function pure and aligned with other make*ErrorMessage() helpers in this file, making reuse possible.
Also decreases arity by 2.

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:174
> +        functionName ? interfaceName : "the ",

I find this approach more readable then prefix/suffix + empty strings.

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:177
> +        " must be ", strings...

While we currently accept 2 arguments max, lets make this a variadic template to make error messages even more customizable.
Chrome & Firefox mention "callback" in error messages.

[Sam Weinig]

Comment on attachment 403024 [details]
Patch

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

Nice improvement.

>> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:174
>> +        functionName ? interfaceName : "the ",
> 
> I find this approach more readable then prefix/suffix + empty strings.

I wonder if there is a nicer alternative. something akin to:

return makeString("Argument ", argumentIndex + 1, " ('", argumentName, "') to ", functionName ? substring("the ", interfaceName, " constructor") : substring(interfaceName, ".", functionName), " must be ", strings...);


where makeSubstring(...) is implemented as something like:

template<typename... Args>
auto makeSubstring(Args... args) -> decltype(auto)
{
     return make_tuple(std::forward<Args...>(args...));
}

and then a general StringTypeAdapter<std::tuple<T...>> implementation to go along with it.

[Alexey Shvayka]

Created attachment 403228 [details]
Patch

Fix `expectedValues` being passed twice and implement StringTypeAdapter<std::tuple>.

[Alexey Shvayka]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 403024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403024&action=review
> 
> Nice improvement.

Entirely Darin's idea, I must say.

> template<typename... Args>
> auto makeSubstring(Args... args) -> decltype(auto)
> {
>      return make_tuple(std::forward<Args...>(args...));
> }

Using std::tuple imposes certain limitations: seems like we can't do `makeSubstring("a", 'b')` or `x ? makeSubstring("a") : makeSubstring("a", "b")`.
Also, I think an API should indicate that no actual string is created, so I went with inline std::make_tuple() + overload. Am I missing something?

[Sam Weinig]

(In reply to Alexey Shvayka from comment #5)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 403024 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403024&action=review
> > 
> > Nice improvement.
> 
> Entirely Darin's idea, I must say.
> 
> > template<typename... Args>
> > auto makeSubstring(Args... args) -> decltype(auto)
> > {
> >      return make_tuple(std::forward<Args...>(args...));
> > }
> 
> Using std::tuple imposes certain limitations: seems like we can't do
> `makeSubstring("a", 'b')` or `x ? makeSubstring("a") : makeSubstring("a",
> "b")`.
> Also, I think an API should indicate that no actual string is created, so I
> went with inline std::make_tuple() + overload. Am I missing something?

I don't think you were missing anything. makeSubstring() was really just aliasing make_tuple (the name makeSubstring is definitely not great, as you note, it doesn't really impart that it is not actually creating a String). Your way seems good.

[Darin Adler]

Comment on attachment 403228 [details]
Patch

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

> Source/WTF/ChangeLog:17
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::StringTypeAdapter):
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::length const):
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::is8Bit const):
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::writeTo const):
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::computeLength):
> +        (WTF::StringTypeAdapter<std::tuple<StringTypes::computeIs8Bit):

The script didn’t do a good job of generating this. I suggest removing it or repairing it so it makes sense.

> Source/WTF/wtf/text/StringConcatenate.h:200
> +template<typename... StringTypes> class StringTypeAdapter<std::tuple<StringTypes...>, void> {

I think there’s a way to do this more generically for all tuple-like things rather than just std::tuple, but we can return and do that later.

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:174
> +        functionName ? std::make_tuple(interfaceName, ".", functionName) : std::make_tuple("the ", interfaceName, " constructor"),

So this only works because both tuples are the same size and have the same types? That seems not really in the spirit of what we’re hoping to achieve, but fine for now.

[Alexey Shvayka]

Created attachment 403233 [details]
Patch

Fix ChangeLog manually.

[Alexey Shvayka]

(In reply to Darin Adler from comment #7)
> So this only works because both tuples are the same size and have the same
> types? That seems not really in the spirit of what we’re hoping to achieve,
> but fine for now.

Yes, otherwise it's compile-time error. I'm not sure how to fix it, or at least make empty arguments default to empty strings.

[EWS]

Committed r263795: <https://trac.webkit.org/changeset/263795>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403233 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64980827>

[Darin Adler]

Comment on attachment 403233 [details]
Patch

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

A few *small* refinement ideas for follow-up later.

> Source/WTF/wtf/text/StringConcatenate.h:200
> +template<typename... StringTypes> class StringTypeAdapter<std::tuple<StringTypes...>, void> {

Refinement ideas for future:

Would like to figure out how to do this for the broader definition of Tuple, anything that supports std::apply, instead of literally std::tuple. I think this is a recurring pattern in template meta-programming, but I don’t know the exact technique.

> Source/WTF/wtf/text/StringConcatenate.h:216
> +                StringTypeAdapter<StringTypes>(strings).writeTo(destination + (offset * sizeof(CharacterType))),

I would have left off the parentheses around the "offset * sizeof(CharacterType)".

> Source/WTF/wtf/text/StringConcatenate.h:231
> +    static unsigned computeLength(StringTypes... strings)
> +    {
> +        return (... + StringTypeAdapter<StringTypes>(strings).length());
> +    }
> +
> +    static bool computeIs8Bit(StringTypes... strings)
> +    {
> +        return (... && StringTypeAdapter<StringTypes>(strings).is8Bit());
> +    }

Refinement ideas for future:

Should we use lambdas for this instead of separate functions, or would this make the constructor too ugly?

Are the parentheses needed? Some coding styles use parentheses with return a lot, but in WebKit we don’t ever use them. I was confused about the "(..." a bit.

[Alexey Shvayka]

(In reply to Darin Adler from comment #12)
> Are the parentheses needed? Some coding styles use parentheses with return a
> lot, but in WebKit we don’t ever use them. I was confused about the "(..." a
> bit.

Please note this is a C++17 fold expression: https://en.cppreference.com/w/cpp/language/fold.
Parentheses are syntax requirement; I've confirmed it doesn't compile w/o them.

[Darin Adler]

(In reply to Alexey Shvayka from comment #13)
> Please note this is a C++17 fold expression:
> https://en.cppreference.com/w/cpp/language/fold.
> Parentheses are syntax requirement; I've confirmed it doesn't compile w/o
> them.

Thanks; great to learn about it!