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");
Created attachment 372457 [details] Basic Sketch of the implementation (not tested)
Attached the basic sketch of the idea. Goal is to reuse as much of the makeString infrastructure as possible.
I'm not wedded to the name. Perhaps a better name would be appendStrings(...).
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.
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.
(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.
Created attachment 373727 [details] Patch
Created attachment 373774 [details] Patch
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.
Created attachment 373782 [details] Patch
(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.
Comment on attachment 373782 [details] Patch Clearing flags on attachment: 373782 Committed r247286: <https://trac.webkit.org/changeset/247286>
All reviewed patches have been landed. Closing bug.
<rdar://problem/52859522>
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
(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.
Reverted r247286 for reason: Caused TestWTF.WTF.StringOperators to fail on debug bots Committed r247307: <https://trac.webkit.org/changeset/247307>
Created attachment 374303 [details] Patch
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.
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
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
Created attachment 374315 [details] Patch
Created attachment 374321 [details] Patch
Comment on attachment 374321 [details] Patch Clearing flags on attachment: 374321 Committed r247537: <https://trac.webkit.org/changeset/247537>