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

Description Sam Weinig 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");
Comment 1 Sam Weinig 2019-06-19 06:12:10 PDT 
Created attachment 372457 [details]
Basic Sketch of the implementation (not tested)
Comment 2 Sam Weinig 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.
Comment 3 Sam Weinig 2019-06-19 06:14:18 PDT 
I'm not wedded to the name. Perhaps a better name would be appendStrings(...).
Comment 4 Darin Adler 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.
Comment 5 Darin Adler 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.
Comment 6 Sam Weinig 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.
Comment 7 Sam Weinig 2019-07-09 09:56:24 PDT 
Created attachment 373727 [details]
Patch
Comment 8 Sam Weinig 2019-07-09 15:08:58 PDT 
Created attachment 373774 [details]
Patch
Comment 9 Darin Adler 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.
Comment 10 Sam Weinig 2019-07-09 15:31:42 PDT 
Created attachment 373782 [details]
Patch
Comment 11 Sam Weinig 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.
Comment 12 WebKit Commit Bot 2019-07-09 16:54:14 PDT Comment hidden (obsolete)
Comment 13 WebKit Commit Bot 2019-07-09 16:54:16 PDT Comment hidden (obsolete)
Comment 14 Radar WebKit Bug Importer 2019-07-09 16:58:14 PDT Comment hidden (obsolete)
Comment 15 Ryan Haddad 2019-07-10 09:47:18 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2019-07-10 09:56:44 PDT Comment hidden (obsolete)
Comment 17 Ryan Haddad 2019-07-10 10:09:11 PDT Comment hidden (obsolete)
Comment 18 Sam Weinig 2019-07-17 10:07:28 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-07-17 10:09:16 PDT Comment hidden (obsolete)
Comment 20 WebKit Commit Bot 2019-07-17 10:58:57 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2019-07-17 12:08:31 PDT Comment hidden (obsolete)
Comment 22 Sam Weinig 2019-07-17 12:31:23 PDT Comment hidden (obsolete)
Comment 23 Sam Weinig 2019-07-17 13:24:02 PDT 
Created attachment 374321 [details]
Patch
Comment 24 WebKit Commit Bot 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>
Comment 25 WebKit Commit Bot 2019-07-17 14:18:38 PDT 
All reviewed patches have been landed.  Closing bug.