Added StringPiece overloads for base::JoinString.

Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241
Review-Url: https://codereview.chromium.org/2691193002
Cr-Commit-Position: refs/heads/master@{#452432}
Committed: https://chromium.googlesource.com/chromium/src/+/b064312df4d19ca62f470d216fb42f479a5801aa

[Matt Giuca, 2017-02-14 03:53:16]

mgiuca@chromium.org changed reviewers:
+ danakj@chromium.org

[Matt Giuca, 2017-02-14 03:53:16]

Hi Dana,

I think this is useful and can't see why there shouldn't be a
base::JoinStringPiece (like SplitStringPiece).

An alternative would be to just call it JoinString and overload
string/StringPiece. But that approach results in ambiguity errors if JoinString
is used with an inline initializer list, as in AutofillProfileComparator:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...
So that would have to be replaced with an explicit constructor to
std::vector<base::StringPiece>. Therefore, I opted to give JoinStringPiece a
separate name.

The version Connie wrote in ShareServiceImpl (which I'm deleting) pre-allocates
the string to its final size, by iterating over the vector first. Do you think
it's worth updating JoinString to do this? For now I'm leaving it as-is.

Here is a WIP follow-up CL which shows a bunch of places (not done yet) where we
can use JoinStringPiece:
https://codereview.chromium.org/2695883003

[Matt Giuca, 2017-02-14 03:53:35]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 03:54:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 05:46:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 05:46:04]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-02-14 08:03:31]

Actually, can you think of any way we can actually make an overload for
StringPiece such that the StringPiece one takes priority?

That is, if we had

  std::string JoinString(std::vector<std::string>);
  std::string JoinString(std::vector<StringPiece>);

And you wrote:

  base::JoinString({"one", "two", "three"});

That it would pick the StringPiece overload, instead of complaining that it's
ambiguous? If we could do that then we could avoid having two separate names.

Perhaps we need an explicit third overload that takes an initializer list?

[danakj, 2017-02-16 16:34:32]

> Perhaps we need an explicit third overload that takes an initializer list?

That seems like a good plan yeah. LGTM as is also if you wanna split it up.

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc...
base/strings/string_util.cc:885: sep.AppendToString(&result);
Just throwing it out there, not for this CL, but: It's unfortunate these are
going to grow the string's size 2N-1 times. We could figure out the size of all
the strings appended together, resize() the result to that, and then fill it in.

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.h#...
base/strings/string_util.h:432: // Does the opposite of SplitString().
and SplitStringPiece()

[danakj, 2017-02-16 16:36:21]

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc...
base/strings/string_util.cc:885: sep.AppendToString(&result);
On 2017/02/16 16:34:32, danakj wrote:
> Just throwing it out there, not for this CL, but: It's unfortunate these are
> going to grow the string's size 2N-1 times. We could figure out the size of
all
> the strings appended together, resize() the result to that, and then fill it
in.

Ohh I see actually at the site that you deleted they used reserve(). I didn't
realize string has that. We could use that for all join implementations.

[dcheng, 2017-02-16 16:38:41]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-02-16 16:38:41]

1) For things that want to allow StringPiece and std::string params, it's more
typical to overload as std::string and const char*. So I think you could do the
same here.

2) That being said, it seems like std::vector<const char*> is pretty rare and
that most instances are really just compile-time arrays. So why doesn't
initializer_list work here?

[dcheng, 2017-02-16 16:58:22]

On 2017/02/16 16:38:41, dcheng wrote:
> 1) For things that want to allow StringPiece and std::string params, it's more
> typical to overload as std::string and const char*. So I think you could do
the
> same here.
> 
> 2) That being said, it seems like std::vector<const char*> is pretty rare and
> that most instances are really just compile-time arrays. So why doesn't
> initializer_list work here?

OK, danakj points out I didn't actually read the CL. Since we already have
SplitStringPiece, I guess it makes sense to allow JoinStringPiece

This all being said... I am a little surprised that we don't just do the simple
thing in web share and string::replace (or have some sort of string templating
helper).

[Matt Giuca, 2017-02-16 23:51:12]

On 2017/02/16 16:38:41, dcheng wrote:
> 1) For things that want to allow StringPiece and std::string params, it's more
> typical to overload as std::string and const char*. So I think you could do
the
> same here.

Do you mean StringPiece and const char*? (Otherwise there'd be no way to pass a
StringPiece without copying.)

I think the issue is that it's in a vector, so we can't rely on implicit
conversion like we would if it was a bare argument. Implicit conversion works
for a literal list, but not if you are dynamically building a vector<string> or
a vector<StringPiece> -- in both of those cases you need a dedicated function
that takes a vector<string> or vector<StringPiece>, respectively.

> 2) That being said, it seems like std::vector<const char*> is pretty rare and
> that most instances are really just compile-time arrays. So why doesn't
> initializer_list work here?

Actually the overwhelming majority of instances are dynamically generated lists
of strings (some, but not most, of which can be more efficiently expressed as
StringPieces).

> OK, danakj points out I didn't actually read the CL. Since we already have
> SplitStringPiece, I guess it makes sense to allow JoinStringPiece

I'm not sure SplitStringPiece warrants a separately-named JoinStringPiece...
SplitString and SplitStringPiece need separate names because they only differ in
the output type (can't be overloaded). I think if we could find a way to
overload JoinString/JoinStringPiece where my example in comment #7 works
unambiguously, that would be preferable to having separate names. I will try the
initializer_list approach and see if it works. (Let me get back to you.)

> 
> This all being said... I am a little surprised that we don't just do the
simple
> thing in web share and string::replace (or have some sort of string templating
> helper).

Haha, you have no idea how many times we iterated on this (writing
ReplacePlaceholders was probably the most nit-picked and rewritten part of the
entire WebShare implementation):

Here's an older version that used string::replace:
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar...

We wanted to be able to do the replacement operation in linear time. We didn't
find an existing appropriate string template substitution function.

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc...
base/strings/string_util.cc:885: sep.AppendToString(&result);
On 2017/02/16 16:36:21, danakj wrote:
> On 2017/02/16 16:34:32, danakj wrote:
> > Just throwing it out there, not for this CL, but: It's unfortunate these are
> > going to grow the string's size 2N-1 times. We could figure out the size of
> all
> > the strings appended together, resize() the result to that, and then fill it
> in.
> 
> Ohh I see actually at the site that you deleted they used reserve(). I didn't
> realize string has that. We could use that for all join implementations.

Yes! Maybe you missed this in my original email:

> The version Connie wrote in ShareServiceImpl (which I'm deleting)
pre-allocates
> the string to its final size, by iterating over the vector first. Do you think
> it's worth updating JoinString to do this? For now I'm leaving it as-is.

I think we should do it -- it's probably faster but there is a performance
trade-off here (iterating over the vector twice) so we maybe should profile it
first. I'll do that as a separate CL.

[danakj, 2017-02-16 23:53:29]

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.cc...
base/strings/string_util.cc:885: sep.AppendToString(&result);
On 2017/02/16 23:51:11, Matt Giuca wrote:
> On 2017/02/16 16:36:21, danakj wrote:
> > On 2017/02/16 16:34:32, danakj wrote:
> > > Just throwing it out there, not for this CL, but: It's unfortunate these
are
> > > going to grow the string's size 2N-1 times. We could figure out the size
of
> > all
> > > the strings appended together, resize() the result to that, and then fill
it
> > in.
> > 
> > Ohh I see actually at the site that you deleted they used reserve(). I
didn't
> > realize string has that. We could use that for all join implementations.
> 
> Yes! Maybe you missed this in my original email:
> 
> > The version Connie wrote in ShareServiceImpl (which I'm deleting)
> pre-allocates
> > the string to its final size, by iterating over the vector first. Do you
think
> > it's worth updating JoinString to do this? For now I'm leaving it as-is.
> 
> I think we should do it -- it's probably faster but there is a performance
> trade-off here (iterating over the vector twice) so we maybe should profile it
> first. I'll do that as a separate CL.

Oops, ya I missed that. I super doubt walking the vector and calling size can
compare to calling realloc even one time. But we should do it in both methods
not just the StringPiece one, so ya let's do it as a separate CL.

[dcheng, 2017-02-17 03:55:54]

On 2017/02/16 23:51:12, Matt Giuca wrote:
> On 2017/02/16 16:38:41, dcheng wrote:
> > 1) For things that want to allow StringPiece and std::string params, it's
more
> > typical to overload as std::string and const char*. So I think you could do
> the
> > same here.
> 
> Do you mean StringPiece and const char*? (Otherwise there'd be no way to pass
a
> StringPiece without copying.)
> 
> I think the issue is that it's in a vector, so we can't rely on implicit
> conversion like we would if it was a bare argument. Implicit conversion works
> for a literal list, but not if you are dynamically building a vector<string>
or
> a vector<StringPiece> -- in both of those cases you need a dedicated function
> that takes a vector<string> or vector<StringPiece>, respectively.

Right, so with const char* and std::string, you don't rely on implicit
conversions at all. But as noted later, that doesn't work, because we don't
actually have a const char*.

> 
> > 2) That being said, it seems like std::vector<const char*> is pretty rare
and
> > that most instances are really just compile-time arrays. So why doesn't
> > initializer_list work here?
> 
> Actually the overwhelming majority of instances are dynamically generated
lists
> of strings (some, but not most, of which can be more efficiently expressed as
> StringPieces).
> 
> > OK, danakj points out I didn't actually read the CL. Since we already have
> > SplitStringPiece, I guess it makes sense to allow JoinStringPiece
> 
> I'm not sure SplitStringPiece warrants a separately-named JoinStringPiece...
> SplitString and SplitStringPiece need separate names because they only differ
in
> the output type (can't be overloaded). I think if we could find a way to
> overload JoinString/JoinStringPiece where my example in comment #7 works
> unambiguously, that would be preferable to having separate names. I will try
the
> initializer_list approach and see if it works. (Let me get back to you.)

I don't feel strongly, but it's kind of nice to having naming symmetry.

Note that initializer list really only works well if you're initializing with a
constant-size list of "stuff". There's some gotchas when that's not the case
(see
https://akrzemi1.wordpress.com/2016/07/07/the-cost-of-stdinitializer_list/). So
I'm OK with vector<StringPiece> here.

> 
> > 
> > This all being said... I am a little surprised that we don't just do the
> simple
> > thing in web share and string::replace (or have some sort of string
templating
> > helper).
> 
> Haha, you have no idea how many times we iterated on this (writing
> ReplacePlaceholders was probably the most nit-picked and rewritten part of the
> entire WebShare implementation):
> 
> Here's an older version that used string::replace:
>
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar...
> 
> We wanted to be able to do the replacement operation in linear time. We didn't
> find an existing appropriate string template substitution function.

Given the length of the strings (there's a limit, right? =), I feel it probably
doesn't matter that much. And the result is very obvious with string::replace.
*shrug*

[Matt Giuca, 2017-02-17 04:29:05]

On 2017/02/17 03:55:54, dcheng wrote:
> On 2017/02/16 23:51:12, Matt Giuca wrote:
> > On 2017/02/16 16:38:41, dcheng wrote:
> > > 1) For things that want to allow StringPiece and std::string params, it's
> more
> > > typical to overload as std::string and const char*. So I think you could
do
> > the
> > > same here.
> > 
> > Do you mean StringPiece and const char*? (Otherwise there'd be no way to
pass
> a
> > StringPiece without copying.)
> > 
> > I think the issue is that it's in a vector, so we can't rely on implicit
> > conversion like we would if it was a bare argument. Implicit conversion
works
> > for a literal list, but not if you are dynamically building a vector<string>
> or
> > a vector<StringPiece> -- in both of those cases you need a dedicated
function
> > that takes a vector<string> or vector<StringPiece>, respectively.
> 
> Right, so with const char* and std::string, you don't rely on implicit
> conversions at all. But as noted later, that doesn't work, because we don't
> actually have a const char*.
> 
> > 
> > > 2) That being said, it seems like std::vector<const char*> is pretty rare
> and
> > > that most instances are really just compile-time arrays. So why doesn't
> > > initializer_list work here?
> > 
> > Actually the overwhelming majority of instances are dynamically generated
> lists
> > of strings (some, but not most, of which can be more efficiently expressed
as
> > StringPieces).
> > 
> > > OK, danakj points out I didn't actually read the CL. Since we already have
> > > SplitStringPiece, I guess it makes sense to allow JoinStringPiece
> > 
> > I'm not sure SplitStringPiece warrants a separately-named JoinStringPiece...
> > SplitString and SplitStringPiece need separate names because they only
differ
> in
> > the output type (can't be overloaded). I think if we could find a way to
> > overload JoinString/JoinStringPiece where my example in comment #7 works
> > unambiguously, that would be preferable to having separate names. I will try
> the
> > initializer_list approach and see if it works. (Let me get back to you.)
> 
> I don't feel strongly, but it's kind of nice to having naming symmetry.

The symmetry is nice, but if we are able to have JoinString work for both (with
the same name) then people might accidentally use the more efficient one, which
is nice. I think if possible we should add the StringPiece version as an
overload, rather than a separately named function.

> Note that initializer list really only works well if you're initializing with
a
> constant-size list of "stuff". There's some gotchas when that's not the case
> (see
> https://akrzemi1.wordpress.com/2016/07/07/the-cost-of-stdinitializer_list/).
So
> I'm OK with vector<StringPiece> here.

My thinking was to use initializer_list as a third overload just for when you
initialize with a constant-size list. The vector<string> and vector<StringPiece>
versions would still be there for when you pass a vector object.

(I'd only do this if it meant the initializer_list fixes the ambiguity mentioned
above... haven't had an opportunity to try yet; sheriffing.)

> 
> > 
> > > 
> > > This all being said... I am a little surprised that we don't just do the
> > simple
> > > thing in web share and string::replace (or have some sort of string
> templating
> > > helper).
> > 
> > Haha, you have no idea how many times we iterated on this (writing
> > ReplacePlaceholders was probably the most nit-picked and rewritten part of
the
> > entire WebShare implementation):
> > 
> > Here's an older version that used string::replace:
> >
>
https://codereview.chromium.org/2564483003/diff/160001/chrome/browser/webshar...
> > 
> > We wanted to be able to do the replacement operation in linear time. We
didn't
> > find an existing appropriate string template substitution function.
> 
> Given the length of the strings (there's a limit, right? =), I feel it
probably
> doesn't matter that much. And the result is very obvious with string::replace.
> *shrug*

Yeah I feel it doesn't matter much either but it's hard to argue that we should
have a O(N^2) algorithm when there's an O(N) algorithm. There could be
pathological cases like a very long URL template with multiple copies of the
same field which would give bad O(N^2) behaviour.

[Matt Giuca, 2017-02-17 05:59:41]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2017-02-17 06:05:42]

Patchset #2 (id:40001) has been deleted

[Matt Giuca, 2017-02-17 06:09:59]

Yay, it works.

I added a third overload which takes an initializer_list<StringPiece> and that
now unambiguously gets selected when you pass a literal list of strings or
StringPieces. Added a test for this also.

WDYT? It's a bit unfortunate that I had to add a third implementation for
initializer_list, but it would otherwise require copying into a vector, so I
think we just have to live with this.

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/1/base/strings/string_util.h#...
base/strings/string_util.h:432: // Does the opposite of SplitString().
On 2017/02/16 16:34:32, danakj wrote:
> and SplitStringPiece()

Done.

[Matt Giuca, 2017-02-17 06:12:11]

Description was changed from

==========
Added base::JoinStringPiece.

Like JoinString but takes a std::vector<StringPiece>.

ShareServiceImpl now uses base::JoinStringPiece instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than the existing JoinString).

BUG=348241
==========

to

==========
Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241
==========

[Matt Giuca, 2017-02-17 06:12:22]

Description was changed from

==========
Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241
==========

to

==========
Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241
==========

[Matt Giuca, 2017-02-17 06:14:31]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 06:15:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 08:59:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 08:59:37]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-02-17 09:51:22]

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
base/strings/string_util.cc:894: const
std::initializer_list<BasicStringPiece<STR>>& parts,
Upon further thought, I can probably template this and avoid duplicating. I'll
try this on Monday (sorry for so many revisions).

[Matt Giuca, 2017-02-20 06:10:55]

Patchset #7 (id:160001) has been deleted

[Matt Giuca, 2017-02-20 23:55:41]

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
base/strings/string_util.cc:894: const
std::initializer_list<BasicStringPiece<STR>>& parts,
On 2017/02/17 09:51:22, Matt Giuca wrote:
> Upon further thought, I can probably template this and avoid duplicating. I'll
> try this on Monday (sorry for so many revisions).

Done.

I managed to combine all 3 into one version you can see in Patch Set 6 (casting
each item to a StringPiece for appending -- this allows it to work on either
StringPieces or strings). I had a brief look at the generated assembly code (on
Linux) and it seems a bit inefficient as it has to call the StringPiece
constructor and AppendToString method, and those won't be inlined without
link-time optimization.

So I would rather not rely on LTO for something in base, and just go with two
separate versions -- Patch Set 7. WDYT?

[danakj, 2017-02-21 23:47:15]

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
base/strings/string_util.cc:894: const
std::initializer_list<BasicStringPiece<STR>>& parts,
On 2017/02/20 23:55:41, Matt Giuca wrote:
> On 2017/02/17 09:51:22, Matt Giuca wrote:
> > Upon further thought, I can probably template this and avoid duplicating.
I'll
> > try this on Monday (sorry for so many revisions).
> 
> Done.
> 
> I managed to combine all 3 into one version you can see in Patch Set 6
(casting
> each item to a StringPiece for appending -- this allows it to work on either
> StringPieces or strings). I had a brief look at the generated assembly code
(on
> Linux) and it seems a bit inefficient as it has to call the StringPiece
> constructor and AppendToString method, and those won't be inlined without
> link-time optimization.
> 
> So I would rather not rely on LTO for something in base, and just go with two
> separate versions -- Patch Set 7. WDYT?

I would say let's avoid temporary objects. You could do this by making an
overloaded helper append function and still keep the rest in a template, for
instance (since string and StringPiece don't share apis).

[Matt Giuca, 2017-02-22 00:10:15]

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
base/strings/string_util.cc:894: const
std::initializer_list<BasicStringPiece<STR>>& parts,
On 2017/02/21 23:47:15, danakj wrote:
> On 2017/02/20 23:55:41, Matt Giuca wrote:
> > On 2017/02/17 09:51:22, Matt Giuca wrote:
> > > Upon further thought, I can probably template this and avoid duplicating.
> I'll
> > > try this on Monday (sorry for so many revisions).
> > 
> > Done.
> > 
> > I managed to combine all 3 into one version you can see in Patch Set 6
> (casting
> > each item to a StringPiece for appending -- this allows it to work on either
> > StringPieces or strings). I had a brief look at the generated assembly code
> (on
> > Linux) and it seems a bit inefficient as it has to call the StringPiece
> > constructor and AppendToString method, and those won't be inlined without
> > link-time optimization.
> > 
> > So I would rather not rely on LTO for something in base, and just go with
two
> > separate versions -- Patch Set 7. WDYT?
> 
> I would say let's avoid temporary objects.

What temporary object do you mean? (Are you saying my current version has a
temporary somewhere I'm not seeing? Or are you referring to Patch Set 6?)

> You could do this by making an
> overloaded helper append function and still keep the rest in a template, for
> instance (since string and StringPiece don't share apis).

Nice suggestion. Will address this later but just sending this out now to
confirm: you're suggesting a way to make Patch Set 6 efficient (while keeping
the single implementation), as opposed to saying there's some inefficiency in
Patch Set 7?

[Matt Giuca, 2017-02-22 05:34:20]

Patchset #8 (id:200001) has been deleted

[Matt Giuca, 2017-02-22 06:20:13]

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_ut...
base/strings/string_util.cc:894: const
std::initializer_list<BasicStringPiece<STR>>& parts,
On 2017/02/22 00:10:15, Matt Giuca wrote:
> On 2017/02/21 23:47:15, danakj wrote:
> > On 2017/02/20 23:55:41, Matt Giuca wrote:
> > > On 2017/02/17 09:51:22, Matt Giuca wrote:
> > > > Upon further thought, I can probably template this and avoid
duplicating.
> > I'll
> > > > try this on Monday (sorry for so many revisions).
> > > 
> > > Done.
> > > 
> > > I managed to combine all 3 into one version you can see in Patch Set 6
> > (casting
> > > each item to a StringPiece for appending -- this allows it to work on
either
> > > StringPieces or strings). I had a brief look at the generated assembly
code
> > (on
> > > Linux) and it seems a bit inefficient as it has to call the StringPiece
> > > constructor and AppendToString method, and those won't be inlined without
> > > link-time optimization.
> > > 
> > > So I would rather not rely on LTO for something in base, and just go with
> two
> > > separate versions -- Patch Set 7. WDYT?
> > 
> > I would say let's avoid temporary objects.
> 
> What temporary object do you mean? (Are you saying my current version has a
> temporary somewhere I'm not seeing? Or are you referring to Patch Set 6?)
> 
> > You could do this by making an
> > overloaded helper append function and still keep the rest in a template, for
> > instance (since string and StringPiece don't share apis).
> 
> Nice suggestion. Will address this later but just sending this out now to
> confirm: you're suggesting a way to make Patch Set 6 efficient (while keeping
> the single implementation), as opposed to saying there's some inefficiency in
> Patch Set 7?

Done.

[dcheng, 2017-02-22 07:22:18]

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.h:436: // Prefer the StringPiece variant if possible,
to avoid unnecessary copying.
I don't think it prevents copies though? It all depends on what you already have
as your input type: if you already have std::string objects, converting to
StringPiece won't help.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.h:449: const std::initializer_list<StringPiece>& parts,
FWIW, it seems much more common to pass by value (as this is just a pointer +
size object, like StringPiece)

[danakj, 2017-02-22 15:10:46]

LGTM % dcheng

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:68: // separate overload for |source| as both
stringpiece and string allows for more
StringPiece

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:75: template <typename STR>
style: blank line between functions

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:873: template <typename LIST, typename STR>
It's not normal in chromium for template types to be ALLCAPS like this. I see
that STR came this way, but maybe we can stop doing that now.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:887: // on both strings and stringpieces without
creating an intermediate
StringPieces

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:888: // stringpiece object.
StringPiece

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.h:436: // Prefer the StringPiece variant if possible,
to avoid unnecessary copying.
On 2017/02/22 07:22:18, dcheng wrote:
> I don't think it prevents copies though? It all depends on what you already
have
> as your input type: if you already have std::string objects, converting to
> StringPiece won't help.

Right, the setup code to make the vector may have less copies, but that's not
related to this API per se.

[Matt Giuca, 2017-02-22 22:39:36]

All the other things I will fix, just not at a computer right now.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:68: // separate overload for |source| as both
stringpiece and string allows for more
On 2017/02/22 15:10:45, danakj wrote:
> StringPiece

I was trying to consistently use lowercase "stringpiece" to refer to "any type
of stringpiece (StringPiece or StringPiece16)". I guess that isn't clear though.
Should I use "BasicStringPiece" instead? Or just "StringPiece" by itself, with
SP16 implied?

[danakj, 2017-02-22 22:40:30]

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:68: // separate overload for |source| as both
stringpiece and string allows for more
On 2017/02/22 22:39:36, Matt Giuca wrote:
> On 2017/02/22 15:10:45, danakj wrote:
> > StringPiece
> 
> I was trying to consistently use lowercase "stringpiece" to refer to "any type
> of stringpiece (StringPiece or StringPiece16)". I guess that isn't clear
though.
> Should I use "BasicStringPiece" instead? Or just "StringPiece" by itself, with
> SP16 implied?

SP16 implied imo. lowercase breaks case-sensitive regex/search so I don't like.
BasicStringPiece would be more confusing to read.

[Matt Giuca, 2017-02-23 04:48:06]

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:68: // separate overload for |source| as both
stringpiece and string allows for more
On 2017/02/22 22:40:30, danakj wrote:
> On 2017/02/22 22:39:36, Matt Giuca wrote:
> > On 2017/02/22 15:10:45, danakj wrote:
> > > StringPiece
> > 
> > I was trying to consistently use lowercase "stringpiece" to refer to "any
type
> > of stringpiece (StringPiece or StringPiece16)". I guess that isn't clear
> though.
> > Should I use "BasicStringPiece" instead? Or just "StringPiece" by itself,
with
> > SP16 implied?
> 
> SP16 implied imo. lowercase breaks case-sensitive regex/search so I don't
like.
> BasicStringPiece would be more confusing to read.

Done.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:75: template <typename STR>
On 2017/02/22 15:10:45, danakj wrote:
> style: blank line between functions

Done.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:873: template <typename LIST, typename STR>
On 2017/02/22 15:10:45, danakj wrote:
> It's not normal in chromium for template types to be ALLCAPS like this. I see
> that STR came this way, but maybe we can stop doing that now.

Done.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:887: // on both strings and stringpieces without
creating an intermediate
On 2017/02/22 15:10:46, danakj wrote:
> StringPieces

Done.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.cc:888: // stringpiece object.
On 2017/02/22 15:10:45, danakj wrote:
> StringPiece

Done.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.h:436: // Prefer the StringPiece variant if possible,
to avoid unnecessary copying.
On 2017/02/22 15:10:46, danakj wrote:
> On 2017/02/22 07:22:18, dcheng wrote:
> > I don't think it prevents copies though? It all depends on what you already
> have
> > as your input type: if you already have std::string objects, converting to
> > StringPiece won't help.
> 
> Right, the setup code to make the vector may have less copies, but that's not
> related to this API per se.

Right, JoinString doesn't create any more/less copies based on which overload.

The point I'm trying to get across is that the caller should be structured to
take advantage of StringPiece where possible, so you avoid copying in the caller
before calling JoinString. I rewrote this to be a bit clearer about that, but
it's also quite verbose. Do you think I should say this? Happy to just delete
the paragraph also.

https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_ut...
base/strings/string_util.h:449: const std::initializer_list<StringPiece>& parts,
On 2017/02/22 07:22:18, dcheng wrote:
> FWIW, it seems much more common to pass by value (as this is just a pointer +
> size object, like StringPiece)

Done.

[dcheng, 2017-02-23 06:32:28]

lgtm

https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_ut...
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_ut...
base/strings/string_util.h:440: // then JoinString, use SplitStringPiece so that
you never have to create copies
Nit: it's nice to avoid pronouns like "you" in comments when possible.

[Matt Giuca, 2017-02-23 06:51:33]

https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_ut...
File base/strings/string_util.h (right):

https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_ut...
base/strings/string_util.h:440: // then JoinString, use SplitStringPiece so that
you never have to create copies
On 2017/02/23 06:32:28, dcheng wrote:
> Nit: it's nice to avoid pronouns like "you" in comments when possible.

Done.

[Matt Giuca, 2017-02-23 06:58:54]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-02-23 06:58:55]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2691193002/#ps300001
(title: "Use default constructor instead of empty string quotes.")

[commit-bot: I haz the power, 2017-02-23 06:59:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 08:41:44]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487833134773210,
"parent_rev": "4cd293b2e7387f6085c1b188aea86bd744fd9f69", "commit_rev":
"b064312df4d19ca62f470d216fb42f479a5801aa"}

[commit-bot: I haz the power, 2017-02-23 08:42:21]

Description was changed from

==========
Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241
==========

to

==========
Added StringPiece overloads for base::JoinString.

The new overloads take std::vector<StringPiece> and
std::initializer_list<StringPiece>. The former allows you to pass a
vector of StringPieces without copying them into strings. The latter is
now required to break ambiguity when used with a literal initializer
list (otherwise the compiler would not be able to decide between the two
other overloads).

ShareServiceImpl now uses base::JoinString instead of its own local
version. I am moving it to base because it is widely applicable (in many
cases more efficient than JoinString(vector<string>)).

BUG=348241

Review-Url: https://codereview.chromium.org/2691193002
Cr-Commit-Position: refs/heads/master@{#452432}
Committed:
https://chromium.googlesource.com/chromium/src/+/b064312df4d19ca62f470d216fb4...
==========

[commit-bot: I haz the power, 2017-02-23 08:42:22]

Committed patchset #12 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/b064312df4d19ca62f470d216fb4...