Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2691193002: Added StringPiece overloads for base::JoinString. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by Matt Giuca
Modified:
4 months ago
CC:
chromium-reviews, jshin+watch_chromium.org, vmpstr+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebase. #

Patch Set 3 : Updated documentation. #

Patch Set 4 : Rename JoinStringPiece to JoinString (overload) and add initializer_list overload. #

Patch Set 5 : Document why we have an explicit initializer_list overload. #

Total comments: 5

Patch Set 6 : Combine into a single templated implementation. #

Patch Set 7 : Roll back those changes because this version is less efficient without link-time optimization. #

Patch Set 8 : Rebase. #

Patch Set 9 : Go back to single implementation and overload append function. #

Total comments: 17

Patch Set 10 : Respond to review. #

Total comments: 2

Patch Set 11 : Reword. #

Patch Set 12 : Use default constructor instead of empty string quotes. #

Messages

Total messages: 46 (20 generated)
Matt Giuca
Hi Dana, I think this is useful and can't see why there shouldn't be a ...
4 months, 2 weeks ago (2017-02-14 03:53:16 UTC) #2
Matt Giuca
Actually, can you think of any way we can actually make an overload for StringPiece ...
4 months, 2 weeks ago (2017-02-14 08:03:31 UTC) #7
danakj
> Perhaps we need an explicit third overload that takes an initializer list? That seems ...
4 months, 1 week ago (2017-02-16 16:34:32 UTC) #8
danakj
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#newcode885 base/strings/string_util.cc:885: sep.AppendToString(&result); On 2017/02/16 16:34:32, danakj wrote: > Just throwing ...
4 months, 1 week ago (2017-02-16 16:36:21 UTC) #9
dcheng (OOO Jun 25 - Jul 1)
1) For things that want to allow StringPiece and std::string params, it's more typical to ...
4 months, 1 week ago (2017-02-16 16:38:41 UTC) #11
dcheng (OOO Jun 25 - Jul 1)
On 2017/02/16 16:38:41, dcheng wrote: > 1) For things that want to allow StringPiece and ...
4 months, 1 week ago (2017-02-16 16:58:22 UTC) #12
Matt Giuca
On 2017/02/16 16:38:41, dcheng wrote: > 1) For things that want to allow StringPiece and ...
4 months, 1 week ago (2017-02-16 23:51:12 UTC) #13
danakj
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#newcode885 base/strings/string_util.cc:885: sep.AppendToString(&result); On 2017/02/16 23:51:11, Matt Giuca wrote: > On ...
4 months, 1 week ago (2017-02-16 23:53:29 UTC) #14
dcheng (OOO Jun 25 - Jul 1)
On 2017/02/16 23:51:12, Matt Giuca wrote: > On 2017/02/16 16:38:41, dcheng wrote: > > 1) ...
4 months, 1 week ago (2017-02-17 03:55:54 UTC) #15
Matt Giuca
On 2017/02/17 03:55:54, dcheng wrote: > On 2017/02/16 23:51:12, Matt Giuca wrote: > > On ...
4 months, 1 week ago (2017-02-17 04:29:05 UTC) #16
Matt Giuca
Yay, it works. I added a third overload which takes an initializer_list<StringPiece> and that now ...
4 months, 1 week ago (2017-02-17 06:09:59 UTC) #19
Matt Giuca
https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc#newcode894 base/strings/string_util.cc:894: const std::initializer_list<BasicStringPiece<STR>>& parts, Upon further thought, I can probably ...
4 months, 1 week ago (2017-02-17 09:51:22 UTC) #26
Matt Giuca
https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc#newcode894 base/strings/string_util.cc:894: const std::initializer_list<BasicStringPiece<STR>>& parts, On 2017/02/17 09:51:22, Matt Giuca wrote: ...
4 months, 1 week ago (2017-02-20 23:55:41 UTC) #28
danakj
https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc#newcode894 base/strings/string_util.cc:894: const std::initializer_list<BasicStringPiece<STR>>& parts, On 2017/02/20 23:55:41, Matt Giuca wrote: ...
4 months, 1 week ago (2017-02-21 23:47:15 UTC) #29
Matt Giuca
https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc#newcode894 base/strings/string_util.cc:894: const std::initializer_list<BasicStringPiece<STR>>& parts, On 2017/02/21 23:47:15, danakj wrote: > ...
4 months, 1 week ago (2017-02-22 00:10:15 UTC) #30
Matt Giuca
4 months, 1 week ago (2017-02-22 00:10:17 UTC) #31
Matt Giuca
https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/120001/base/strings/string_util.cc#newcode894 base/strings/string_util.cc:894: const std::initializer_list<BasicStringPiece<STR>>& parts, On 2017/02/22 00:10:15, Matt Giuca wrote: ...
4 months, 1 week ago (2017-02-22 06:20:13 UTC) #33
dcheng (OOO Jun 25 - Jul 1)
https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.h#newcode436 base/strings/string_util.h:436: // Prefer the StringPiece variant if possible, to avoid ...
4 months, 1 week ago (2017-02-22 07:22:18 UTC) #34
danakj
LGTM % dcheng https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc#newcode68 base/strings/string_util.cc:68: // separate overload for |source| as ...
4 months ago (2017-02-22 15:10:46 UTC) #35
Matt Giuca
All the other things I will fix, just not at a computer right now. https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc ...
4 months ago (2017-02-22 22:39:36 UTC) #36
danakj
https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc#newcode68 base/strings/string_util.cc:68: // separate overload for |source| as both stringpiece and ...
4 months ago (2017-02-22 22:40:30 UTC) #37
Matt Giuca
https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/2691193002/diff/240001/base/strings/string_util.cc#newcode68 base/strings/string_util.cc:68: // separate overload for |source| as both stringpiece and ...
4 months ago (2017-02-23 04:48:06 UTC) #38
dcheng (OOO Jun 25 - Jul 1)
lgtm https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_util.h#newcode440 base/strings/string_util.h:440: // then JoinString, use SplitStringPiece so that you ...
4 months ago (2017-02-23 06:32:28 UTC) #39
Matt Giuca
https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/2691193002/diff/260001/base/strings/string_util.h#newcode440 base/strings/string_util.h:440: // then JoinString, use SplitStringPiece so that you never ...
4 months ago (2017-02-23 06:51:33 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2691193002/300001
4 months ago (2017-02-23 06:59:11 UTC) #43
commit-bot: I haz the power
4 months ago (2017-02-23 08:42:22 UTC) #46
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/b064312df4d19ca62f470d216fb4...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589