Bug 147142

Created attachment 257173 [details] Patch EWS run

Created attachment 257179 [details] Patch

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).

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.

Oops, I missed Anders’ comment.

(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.

Created attachment 257536 [details] Patch

(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.

Created attachment 257573 [details] New StringConcatenate.h

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

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).

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.

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.

Created attachment 258102 [details] Patch Uses const-references in multi-parameter tryMakeString(), makeString() versions.

Comment on attachment 258102 [details] Patch Clearing flags on attachment: 258102 Committed r187815: <http://trac.webkit.org/changeset/187815>

All reviewed patches have been landed. Closing bug.

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?

(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

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

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.

Re-opened since this is blocked by bug 147644

Created attachment 284410 [details] WIP patch

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).

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.

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.

Created attachment 296991 [details] patch Address Mark's comments.

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?

Created attachment 297023 [details] patch Break multiline.

Comment on attachment 297023 [details] patch Clearing flags on attachment: 297023 Committed r209761: <http://trac.webkit.org/changeset/209761>

All reviewed patches have been landed. Closing bug.

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