Bug 198997 - Add StringBuilder member function which allows makeString() style variadic argument construction
Summary: Add StringBuilder member function which allows makeString() style variadic ar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-19 06:10 PDT by Sam Weinig
Modified: 2019-07-17 14:18 PDT (History)
10 users (show)

See Also:


Attachments
Basic Sketch of the implementation (not tested) (2.91 KB, patch)
2019-06-19 06:12 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (13.33 KB, patch)
2019-07-09 09:56 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (13.43 KB, patch)
2019-07-09 15:08 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (16.45 KB, patch)
2019-07-09 15:31 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (23.68 KB, patch)
2019-07-17 10:07 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (24.48 KB, patch)
2019-07-17 12:31 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (26.07 KB, patch)
2019-07-17 13:24 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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.