Bug 198997 – Add StringBuilder member function which allows makeString() style variadic argument construction

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sam Weinig]

We should consider adding a new StringBuilder member function which allows makeString() style variadic argument appending. Allowing calls like:

stringBuilder.appendMakeString("hello", 7, stringVariable, "world");

[Sam Weinig]

Created attachment 372457 [details]
Basic Sketch of the implementation (not tested)

[Sam Weinig]

Attached the basic sketch of the idea. Goal is to reuse as much of the makeString infrastructure as possible.

[Sam Weinig]

I'm not wedded to the name. Perhaps a better name would be appendStrings(...).

[Darin Adler]

Since it can be used to append lots of different things, I like the idea of just calling it "append". Separate append functions that take specific types can have separate names, but usually you wouldn’t need to call them!

Unfortunately, I suppose the current function named "append" appends a string specifically. I wonder if we can make the new thing compatible enough that it can just take over the name.

[Darin Adler]

The name should be like "flexibleAppend" and then we can rename the old one and then just make the new one inherit the append name. I assume there would be no runtime cost to calling it with exactly one argument and so the only issue would be if there’s difficulty disambiguating different handling of the same types.

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> The name should be like "flexibleAppend" and then we can rename the old one
> and then just make the new one inherit the append name. I assume there would
> be no runtime cost to calling it with exactly one argument and so the only
> issue would be if there’s difficulty disambiguating different handling of
> the same types.

Yeah, I'd like to ensure there is no overhead to calling with a single argument.

[Sam Weinig]

Created attachment 373727 [details]
Patch

[Sam Weinig]

Created attachment 373774 [details]
Patch

[Darin Adler]

Comment on attachment 373774 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.h:235
> +    template<typename... StringTypes>
> +    void flexibleAppend(StringTypes ...strings);

// FIXME: Rename to append after renaming any overloads of append that take more than one argument.

Also, consider doing in a single line? Also, can we omit the argument name "strings" here? I don’t think it adds anything.

> Source/WTF/wtf/text/StringBuilder.h:356
> +    template <typename CharType> void reallocateBuffer(unsigned requiredLength);

My preferred formatting for this:

    template<typename CharacterType> void reallocationBuffer(unsigned requiredLength);

(similar for below)

> Source/WTF/wtf/text/StringBuilder.h:369
> +    template<typename... StringTypeAdapters>
> +    void flexibleAppendFromAdapters(StringTypeAdapters ...adapters);

Consider doing in a single line? Also, can we omit the argument name "adapters" here? I don’t think it adds anything.

[Sam Weinig]

Created attachment 373782 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 373774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373774&action=review
> 
> > Source/WTF/wtf/text/StringBuilder.h:235
> > +    template<typename... StringTypes>
> > +    void flexibleAppend(StringTypes ...strings);
> 
> // FIXME: Rename to append after renaming any overloads of append that take
> more than one argument.
> 

Done.

> Also, consider doing in a single line? Also, can we omit the argument name
> "strings" here? I don’t think it adds anything.

All done.

> 
> > Source/WTF/wtf/text/StringBuilder.h:356
> > +    template <typename CharType> void reallocateBuffer(unsigned requiredLength);
> 
> My preferred formatting for this:
> 
>     template<typename CharacterType> void reallocationBuffer(unsigned
> requiredLength);
> 
> (similar for below)

Did this for all the template functions in the file.

> 
> > Source/WTF/wtf/text/StringBuilder.h:369
> > +    template<typename... StringTypeAdapters>
> > +    void flexibleAppendFromAdapters(StringTypeAdapters ...adapters);
> 
> Consider doing in a single line? Also, can we omit the argument name
> "adapters" here? I don’t think it adds anything.

All done.

[WebKit Commit Bot]

Comment on attachment 373782 [details]
Patch

Clearing flags on attachment: 373782

Committed r247286: <https://trac.webkit.org/changeset/247286>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52859522>

[Ryan Haddad]

This change has caused TestWTF.WTF.StringOperators to fail on debug bots:
        
        /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:56
        Expected equality of these values:
          2
          wtfStringCopyCount
            Which is: 0
        string + string
        
        
        /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:57
        Expected equality of these values:
          2
          wtfStringCopyCount
            Which is: 0
        string + atomString
        
        
        /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/StringOperators.cpp:58
        Expected equality of these values:
          2
          wtfStringCopyCount
            Which is: 0
        atomString + string


The full diff is here:
https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/builds/4384/steps/run-api-tests/logs/stdio

[Sam Weinig]

(In reply to Ryan Haddad from comment #15)
> This change has caused TestWTF.WTF.StringOperators to fail on debug bots:
>         
>        
> /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/
> StringOperators.cpp:56
>         Expected equality of these values:
>           2
>           wtfStringCopyCount
>             Which is: 0
>         string + string
>         
>         
>        
> /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/
> StringOperators.cpp:57
>         Expected equality of these values:
>           2
>           wtfStringCopyCount
>             Which is: 0
>         string + atomString
>         
>         
>        
> /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WTF/
> StringOperators.cpp:58
>         Expected equality of these values:
>           2
>           wtfStringCopyCount
>             Which is: 0
>         atomString + string
> 
> 
> The full diff is here:
> https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/
> builds/4384/steps/run-api-tests/logs/stdio

Thanks for letting me know. If you could roll it out, I would appreciate it. I can't take a look at the failure until later.

[Ryan Haddad]

Reverted r247286 for reason:

Caused TestWTF.WTF.StringOperators to fail on debug bots

Committed r247307: <https://trac.webkit.org/changeset/247307>

[Sam Weinig]

Created attachment 374303 [details]
Patch

[EWS Watchlist]

Attachment 374303 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:119:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:126:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:128:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:135:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:136:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:137:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:138:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:139:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:140:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/WTFStringUtilities.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 23 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 374303 [details]
Patch

Rejecting attachment 374303 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
/Library/PrivateFrameworks -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks --system-header-prefix=WebKit/ -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -ftemplate-depth=256 -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TransformationMatrix.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TransformationMatrix.dia -c /Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebCore/TransformationMatrix.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TransformationMatrix.o

Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/TestWebKitAPI normal x86_64
    cd /Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI
    export MACOSX_DEPLOYMENT_TARGET=10.13
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -L/Volumes/Data/EWS/WebKit/WebKitBuild/Release -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -F/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/../../WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101300 -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks -filelist /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI.LinkFileList -Xlinker -rpath -Xlinker @loader_path -mmacosx-version-min=10.13 -Xlinker -object_path_lto -Xlinker /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI_lto.o -stdlib=libc++ -fobjc-link-runtime -Wl,-unexported_symbol -Wl,__ZN7testing4Test16TearDownTestCaseEv -Wl,-unexported_symbol -Wl,__ZN7testing4Test13SetUpTestCaseEv -lgtest -force_load /Volumes/Data/EWS/WebKit/WebKitBuild/Release/libTestWebKitAPI.a -framework JavaScriptCore -framework WebKit -lWebCoreTestSupport -framework Cocoa -framework Carbon -framework CoreGraphics -framework CoreLocation -framework CoreText -framework IOKit -lboringssl -licucore -framework LocalAuthentication -framework QuartzCore -framework Security -Xlinker -dependency_info -Xlinker /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI_dependency_info.dat -o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/TestWebKitAPI
ld: warning: directory not found for option '-F/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/../../WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101300'
Undefined symbols for architecture x86_64:
  "_wtfStringCopyCount", referenced from:
      WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void> >(WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void>) in libTestWebKitAPI.a(SecurityOrigin.o)
      void WTF::StringTypeAdapter<WTF::String, void>::writeTo<unsigned short>(unsigned short*) const in libTestWebKitAPI.a(SecurityOrigin.o)
      WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void> >(WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void>) in libTestWebKitAPI.a(URLParserTextEncoding.o)
      void WTF::StringTypeAdapter<WTF::String, void>::writeTo<unsigned short>(unsigned short*) const in libTestWebKitAPI.a(URLParserTextEncoding.o)
      WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void> >(WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void>) in libTestWebKitAPI.a(URLParserTextEncoding.o)
      WTF::String WTF::tryMakeStringFromAdapters<WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<char const*, void> >(WTF::StringTypeAdapter<char const*, void>, WTF::StringTypeAdapter<WTF::String, void>, WTF::StringTypeAdapter<char const*, void>) in libTestWebKitAPI.a(URLParserTextEncoding.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** BUILD FAILED **


The following build commands failed:
	Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/TestWebKitAPI normal x86_64
(1 failure)

Full output: https://webkit-queues.webkit.org/results/12759414

[EWS Watchlist]

Comment on attachment 374303 [details]
Patch

Attachment 374303 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12759489

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests

[Sam Weinig]

Created attachment 374315 [details]
Patch

[Sam Weinig]

Created attachment 374321 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374321 [details]
Patch

Clearing flags on attachment: 374321

Committed r247537: <https://trac.webkit.org/changeset/247537>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.