Bug 198997

Summary: Add StringBuilder member function which allows makeString() style variadic argument construction
Product: WebKit Reporter: Sam Weinig <sam>
Component: Web Template FrameworkAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, ryanhaddad, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Basic Sketch of the implementation (not tested)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Sam Weinig
Reported 2019-06-19 06:10:41 PDT
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");
Attachments
Basic Sketch of the implementation (not tested) (2.91 KB, patch)
2019-06-19 06:12 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (13.33 KB, patch)
2019-07-09 09:56 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (13.43 KB, patch)
2019-07-09 15:08 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (16.45 KB, patch)
2019-07-09 15:31 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (23.68 KB, patch)
2019-07-17 10:07 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (24.48 KB, patch)
2019-07-17 12:31 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (26.07 KB, patch)
2019-07-17 13:24 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Sam Weinig
Comment 1 2019-06-19 06:12:10 PDT
Created attachment 372457 [details] Basic Sketch of the implementation (not tested)
Sam Weinig
Comment 2 2019-06-19 06:12:53 PDT
Attached the basic sketch of the idea. Goal is to reuse as much of the makeString infrastructure as possible.
Sam Weinig
Comment 3 2019-06-19 06:14:18 PDT
I'm not wedded to the name. Perhaps a better name would be appendStrings(...).
Darin Adler
Comment 4 2019-06-19 09:54:46 PDT
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
Comment 5 2019-06-19 09:55:57 PDT
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
Comment 6 2019-06-19 13:48:56 PDT
(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
Comment 7 2019-07-09 09:56:24 PDT
Created attachment 373727 [details] Patch
Sam Weinig
Comment 8 2019-07-09 15:08:58 PDT
Created attachment 373774 [details] Patch
Darin Adler
Comment 9 2019-07-09 15:18:53 PDT
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
Comment 10 2019-07-09 15:31:42 PDT
Created attachment 373782 [details] Patch
Sam Weinig
Comment 11 2019-07-09 15:32:49 PDT
(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 12 2019-07-09 16:54:14 PDT Comment hidden (obsolete)
WebKit Commit Bot
Comment 13 2019-07-09 16:54:16 PDT Comment hidden (obsolete)
Radar WebKit Bug Importer
Comment 14 2019-07-09 16:58:14 PDT Comment hidden (obsolete)
Ryan Haddad
Comment 15 2019-07-10 09:47:18 PDT Comment hidden (obsolete)
Sam Weinig
Comment 16 2019-07-10 09:56:44 PDT Comment hidden (obsolete)
Ryan Haddad
Comment 17 2019-07-10 10:09:11 PDT Comment hidden (obsolete)
Sam Weinig
Comment 18 2019-07-17 10:07:28 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 19 2019-07-17 10:09:16 PDT Comment hidden (obsolete)
WebKit Commit Bot
Comment 20 2019-07-17 10:58:57 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 21 2019-07-17 12:08:31 PDT Comment hidden (obsolete)
Sam Weinig
Comment 22 2019-07-17 12:31:23 PDT Comment hidden (obsolete)
Sam Weinig
Comment 23 2019-07-17 13:24:02 PDT
Created attachment 374321 [details] Patch
WebKit Commit Bot
Comment 24 2019-07-17 14:18:36 PDT
Comment on attachment 374321 [details] Patch Clearing flags on attachment: 374321 Committed r247537: <https://trac.webkit.org/changeset/247537>
WebKit Commit Bot
Comment 25 2019-07-17 14:18:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.