Bug 147142 – [WTF] Turn tryMakeString(), makeString() into variadic templates

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[WTF] Turn tryMakeString(), makeString() into variadic templates

[Zan Dobersek]

Created attachment 257173 [details]
Patch

EWS run

[Zan Dobersek]

Created attachment 257179 [details]
Patch

[Anders Carlsson]

How similar is this to 

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

(Not saying you shouldn't do it - I started doing it but forgot about it since the patch didn't build on OS X/iOS).

[Darin Adler]

Comment on attachment 257179 [details]
Patch

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

> Source/WTF/wtf/text/StringConcatenate.h:330
> +    return String(std::forward<T>(string1));

I’m surprised the explicit cast to String() is needed here.

> Source/WTF/wtf/text/StringOperators.h:37
> +        RefPtr<StringImpl> resultImpl = StringAdapterTuple<2, AdapterType<T1>, AdapterType<T2>>::createString(m_tuple);

Seems like this should be named result, not resultImpl.

[Darin Adler]

Oops, I missed Anders’ comment.

[Zan Dobersek]

(In reply to comment #3)
> How similar is this to 
> 
> https://bugs.webkit.org/attachment.cgi?id=239228&action=review
> 
> (Not saying you shouldn't do it - I started doing it but forgot about it
> since the patch didn't build on OS X/iOS).

It's very similar. I'll try to adopt the use of additional adapters and Checked<>, and update the patch with that.

[Zan Dobersek]

Created attachment 257536 [details]
Patch

[Sam Weinig]

(In reply to comment #7)
> Created attachment 257536 [details]
> Patch

Thanks for picking this up Zan! Would you mind posting the non-diff version of StringConcatenate.h. I think it would make it easier to review.

[Zan Dobersek]

Created attachment 257573 [details]
New StringConcatenate.h

[Sam Weinig]

Comment on attachment 257536 [details]
Patch

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

> Source/WTF/wtf/text/StringConcatenate.h:391
> +String makeString(T1&& string1, T2&& string2, Ts&&... strings)

Why does this take three parameters? Shouldn't this be String makeString(T1&& string,  Ts&&... strings)? Same question for tryMakeString

[Sam Weinig]

Comment on attachment 257536 [details]
Patch

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

>> Source/WTF/wtf/text/StringConcatenate.h:391
>> +String makeString(T1&& string1, T2&& string2, Ts&&... strings)
> 
> Why does this take three parameters? Shouldn't this be String makeString(T1&& string,  Ts&&... strings)? Same question for tryMakeString

I also think these can be const references rather than universal references, since there will never be any stealing going on.  (This is of course not true of the makeString that takes only one String).

[Zan Dobersek]

Comment on attachment 257536 [details]
Patch

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

>>> Source/WTF/wtf/text/StringConcatenate.h:391
>>> +String makeString(T1&& string1, T2&& string2, Ts&&... strings)
>> 
>> Why does this take three parameters? Shouldn't this be String makeString(T1&& string,  Ts&&... strings)? Same question for tryMakeString
> 
> I also think these can be const references rather than universal references, since there will never be any stealing going on.  (This is of course not true of the makeString that takes only one String).

The variadic pack can be of zero size, essentially containing no elements. The first two parameters ensure that at least two arguments are passed in.

[Zan Dobersek]

Comment on attachment 257536 [details]
Patch

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

>>> Source/WTF/wtf/text/StringConcatenate.h:391
>>> +String makeString(T1&& string1, T2&& string2, Ts&&... strings)
>> 
>> Why does this take three parameters? Shouldn't this be String makeString(T1&& string,  Ts&&... strings)? Same question for tryMakeString
> 
> I also think these can be const references rather than universal references, since there will never be any stealing going on.  (This is of course not true of the makeString that takes only one String).

The variadic pack can be of zero size, essentially containing no elements. The first two parameters ensure that at least two arguments are passed in.

[Zan Dobersek]

Created attachment 258102 [details]
Patch

Uses const-references in multi-parameter tryMakeString(), makeString() versions.

[Zan Dobersek]

Comment on attachment 258102 [details]
Patch

Clearing flags on attachment: 258102

Committed r187815: <http://trac.webkit.org/changeset/187815>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Looks like this broke the Windows build:
<https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/90030/steps/compile-webkit/logs/stdio>

A lot of errors that look like:

> C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\wtf/text/StringOperators.h(72): error C2079: 'WTF::StringAppend<const char *,WTF::String>::m_tuple' uses undefined class 'std::tuple<WTF::StringTypeAdapter<const char*>,WTF::StringTypeAdapter<WTF::String>>' (..\wtf\DecimalNumber.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\WTF.vcxproj\WTF.vcxproj]
>           C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\wtf/text/StringOperators.h(97) : see reference to class template instantiation 'WTF::StringAppend<const char *,WTF::String>' being compiled

Have time to investigate? Or should we consider rolling out?

[Zan Dobersek]

(In reply to comment #17)
> Looks like this broke the Windows build:
> <https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> 90030/steps/compile-webkit/logs/stdio>
> 
> A lot of errors that look like:
> 
> > C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\wtf/text/StringOperators.h(72): error C2079: 'WTF::StringAppend<const char *,WTF::String>::m_tuple' uses undefined class 'std::tuple<WTF::StringTypeAdapter<const char*>,WTF::StringTypeAdapter<WTF::String>>' (..\wtf\DecimalNumber.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\WTF.vcxproj\WTF.vcxproj]
> >           C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\wtf/text/StringOperators.h(97) : see reference to class template instantiation 'WTF::StringAppend<const char *,WTF::String>' being compiled
> 
> Have time to investigate? Or should we consider rolling out?

Fixed in r187817.
http://trac.webkit.org/changeset/187817

[Zan Dobersek]

*** Bug 137369 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

A lot of Windows tests are crashing, and this is the most likely cause in the revision range:

https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29?numbuilds=100

Please investigate. I will roll out to test this theory in a few hours.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 147644

[Zan Dobersek]

Created attachment 284410 [details]
WIP patch

[JF Bastien]

Created attachment 296975 [details]
patch

This seems stalled, and I didn't know the bug existed until after writing the code. I propose this minimal patch which *only* makes the code variadic, and I'll be happy to do follow-ups later, including allowing us to makeString from non-string things (e.g. using String::number on numbers, and enabling user-defined stringifiers through ADL).

There are two FIXMEs in there which I'd like to change, either here or in a follow-up (since it's a behavior change).

[JF Bastien]

Comment on attachment 296975 [details]
patch

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

> Source/WTF/wtf/text/StringConcatenate.h:144
> +        if (length > std::numeric_limits<unsigned>::max()) // FIXME

This is sketchy: length is already unsigned. A string that long should probably CRASH anyways as it's a symptom of a deeper problem.

> Source/WTF/wtf/text/StringConcatenate.h:153
> +    NO_RETURN_DUE_TO_CRASH void writeTo(LChar*) const // FIXME

We could fail this if instantiated instead.

[Mark Lam]

Comment on attachment 296975 [details]
patch

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

Some comments for now.  Will read the rest carefully tomorrow when my mind is more awake.

> Source/WTF/ChangeLog:8
> +        I wrote this patch while improving WebAssembly's error messages, and only found this bug afterwads. My implementation does the bare minimum to make this code variadic without changing behavior. I think it's better to go with this baby step first, and improve the code later. Notable, for my WebAssembly patch I also taught the code to handle integers and other types (including WebAssembly types). A follow-up could rely on ADL magic to pretty-format these other types.

/afterwads/afterwards/.

nit: I'd prefer it if you put some line breaks in this and break it into multiple lines.

>> Source/WTF/wtf/text/StringConcatenate.h:144
>> +        if (length > std::numeric_limits<unsigned>::max()) // FIXME
> 
> This is sketchy: length is already unsigned. A string that long should probably CRASH anyways as it's a symptom of a deeper problem.

You should file a bug and include the bug url with the FIXME.

>> Source/WTF/wtf/text/StringConcatenate.h:153
>> +    NO_RETURN_DUE_TO_CRASH void writeTo(LChar*) const // FIXME
> 
> We could fail this if instantiated instead.

Ditto.  File a bug so that we don't forget it.

[JF Bastien]

Created attachment 296991 [details]
patch

Address Mark's comments.

[Mark Lam]

Comment on attachment 296991 [details]
patch

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

r=me

> Source/WTF/ChangeLog:20
> +        (WTF::sumWithOverflow): This unconditionally does the sum for all inputs, which the compiler is more likely to appreciate (because it's the common case) compared to testing for overflow and bailing on each addition

nit: can you add some line breaks to this line too?

[JF Bastien]

Created attachment 297023 [details]
patch

Break multiline.

[WebKit Commit Bot]

Comment on attachment 297023 [details]
patch

Clearing flags on attachment: 297023

Committed r209761: <http://trac.webkit.org/changeset/209761>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

*** Bug 129235 has been marked as a duplicate of this bug. ***