Bug 177566 - Support concatenating StringView with other string types
Summary: Support concatenating StringView with other string types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-27 13:59 PDT by Daniel Bates
Modified: 2018-12-17 12:37 PST (History)
9 users (show)

See Also:


Attachments
Patch and unit tests (31.73 KB, patch)
2017-09-27 14:08 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (28.86 KB, patch)
2018-12-17 12:35 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-09-27 13:59:41 PDT
We should add StringOperators support for StringViews so that stringView + string works. Currently only makeString() is smart enough to concatenate a passed StringView with other string types.
Comment 1 Daniel Bates 2017-09-27 14:08:35 PDT
Created attachment 322014 [details]
Patch and unit tests
Comment 2 Sam Weinig 2017-09-28 12:49:34 PDT
I think it is confusing that we have these two methods of concatenation, operator+ and makeString(). I have been using makeString() under the assumption that only it allowed for a gaurenteed single allocation, no matter how many parameters were passed. It would be good if we could pick one and stick to it.
Comment 3 Daniel Bates 2017-09-28 13:08:37 PDT
(In reply to Sam Weinig from comment #2)
> I think it is confusing that we have these two methods of concatenation,
> operator+ and makeString(). I have been using makeString() under the
> assumption that only it allowed for a gaurenteed single allocation, no
> matter how many parameters were passed. It would be good if we could pick
> one and stick to it.

I agree we should standardize on one way to concatenate a string. StringOperators is syntactic sugar on top of makeString(). I like the syntactic sugar and this motivated me to write this patch. Should I email webkit-dev? How do you suggest that we proceed?
Comment 4 Daniel Bates 2017-09-28 13:38:55 PDT
For completeness, StringOperators was added in the patch for bug #58420 landed in <https://trac.webkit.org/changeset/86330>. From the get-go it was implemented in terms of tryMakeString().
Comment 5 Darin Adler 2017-11-21 09:23:27 PST
Comment on attachment 322014 [details]
Patch and unit tests

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

> Source/WTF/ChangeLog:18
> +        (WTF::StringTypeAdapter<StringView>::StringTypeAdapter<StringView>): Moved from StringView.h.

Why?
Comment 6 Daniel Bates 2018-12-17 12:25:00 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 322014 [details]
> Patch and unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322014&action=review
> 
> > Source/WTF/ChangeLog:18
> > +        (WTF::StringTypeAdapter<StringView>::StringTypeAdapter<StringView>): Moved from StringView.h.
> 
> Why?

Will revert.

It has been a while since I wrote this patch. I suspect the reason I moved this class from file Source/WTF/wtf/text/StringView.h to file StringConcatenate.h was to group it with all the other StringTypeAdapter classes defined in file StringConcatenate.h.
Comment 7 Daniel Bates 2018-12-17 12:29:44 PST
Seeing some of Darin's recent String::format() changes reminded me of this patch. I find the string concatenation functions convenient to use and more aesthetically pleasing than using the variadic function makeString(). I am going to land this patch.
Comment 8 Daniel Bates 2018-12-17 12:35:11 PST
Created attachment 357467 [details]
To Land
Comment 9 Daniel Bates 2018-12-17 12:36:45 PST
Committed r239282: <https://trac.webkit.org/changeset/239282>
Comment 10 Radar WebKit Bug Importer 2018-12-17 12:37:25 PST
<rdar://problem/46786243>