De-duplicate common code from StringPiece, StringPiece16, and their tests.

Extract common code from StringPiece and StringPiece16 into a templated base class. Convert copy-and-pasted unit tests into TYPED_TESTs.

The motivation is that I wish to add constructors for string::iterator ranges and I don't want to do it twice.

The motivation for adding the string::iterator range constructors is to reduce the number of overloads in string_number_conversions.h .

BUG=87634
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114242

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115616

[erikwright (departed), 2011-11-29 19:38:28]

Hi Brett,

Some time ago we spoke about string_to_number_conversions, and removing the
various overloaded signatures in favour of StringPiece. The first step, at the
time, was to implement StringPiece16.

I'm getting back to this now, and I have noticed that someone has already
defined a StringPiece16. So I plan to:

1) add iterator-range constructors for StringPiece and StringPiece16
2) add StringToInt(StringPiece, int*) overloads
3) update callers
4) then remove the other forms of StringToInt

I have noticed that StringPiece16 was written by copy-and-pasting StringPiece.
Rather than update both copies with the iterator-range constructor, I prefer to
refactor them to remove repetition, then add the constructor once.

This CL redefines StringPiece and StringPiece16 in terms of a base class
template that contains the common behaviour.

PTAL.

[brettw, 2011-12-01 20:29:24]

I think this will make the two templates get instantiated in every .o file that
String[16]Piece is used, which will slow down compilation and make everything
larger.

To get around this, we should use extern templates to tell the compiler not to
instantiate it everywhere.

You can see some examples of this in base/logging.h and .cc (the template is
actually instantiated in the .cc file, the .h file declares it extern).

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode36
base/string_piece.h:36: namespace detail {
Normally this would be "internal" or something, but I don't actually think it's
necessary in this case. I think it's fine for this to just be in the base
namespace (just like basic_string is in std).

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode50
base/string_piece.h:50: typedef std::reverse_iterator<iterator>
reverse_iterator;
I'd make this also const to match the non-reverse iterator (everything is a
const iterator in StringPiece).

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode105
base/string_piece.h:105: iterator begin() const { return ptr_; }
Can these return const_iterator instead? The return type will be the same, but I
think it's best if everything matches.

[erikwright (departed), 2011-12-06 15:52:06]

PTAL.

In particular, I fixed an inconsistency where StringPiece was a subclass of
BasicStringPiece whereas StringPiece16 was a direct instantiation. This allows
client code templated on CHAR_TYPE to select the appropriate StringPiece classes
correctly.

This makes it more complicated to forward-declare StringPiece, but this was
already discouraged as it prevents users from automatically coercing string/char
types to StringPiece at call-sites.

I updated all forward declarers, and have uploaded separate CLs for each one.
This CL will only be committed when all the others have landed. So only
base/logging.h and base/string_piece.{h,cc} will be committed in this CL.

Here is a query that lists the related CLs:

http://goo.gl/GuheX

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode36
base/string_piece.h:36: namespace detail {
On 2011/12/01 20:29:24, brettw wrote:
> Normally this would be "internal" or something, but I don't actually think
it's
> necessary in this case. I think it's fine for this to just be in the base
> namespace (just like basic_string is in std).

A compelling reason for basic_string to be in std is because you might template
a client class, method, or function on basic_string<CHAR>. The original CL here
did not really support that, since StringPiece was a subclass of
BasicStringPiece while StringPiece16 was a direct instantiation.

I modified things so that both StringPiece and StringPiece16 are typedef'd to
BasicStringPiece<std::string/string16>, which is itself a subclass of
StringPieceDetail. The common behaviour is in StringPieceDetail while
BasicStringPiece is specialized to add functionality to the single-byte
versions.

With that in mind, I would propose that clients should never refer directly to
StringPieceDetail, and it therefore belongs in an 'internal' namespace.

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode50
base/string_piece.h:50: typedef std::reverse_iterator<iterator>
reverse_iterator;
On 2011/12/01 20:29:24, brettw wrote:
> I'd make this also const to match the non-reverse iterator (everything is a
> const iterator in StringPiece).

In fact, why not just remove the non-const typedef's, since they are not used
(if I follow the logical conclusions of your following comment).

http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode105
base/string_piece.h:105: iterator begin() const { return ptr_; }
On 2011/12/01 20:29:24, brettw wrote:
> Can these return const_iterator instead? The return type will be the same, but
I
> think it's best if everything matches.

Sure. Given this, I guess the next step would be to simply remove iterator and
reverse_iterator from the definition, as they are no longer used?

[erikwright (departed), 2011-12-07 14:54:20]

On 2011/12/06 15:52:06, erikwright wrote:
> PTAL.
> 
> In particular, I fixed an inconsistency where StringPiece was a subclass of
> BasicStringPiece whereas StringPiece16 was a direct instantiation. This allows
> client code templated on CHAR_TYPE to select the appropriate StringPiece
classes
> correctly.
> 
> This makes it more complicated to forward-declare StringPiece, but this was
> already discouraged as it prevents users from automatically coercing
string/char
> types to StringPiece at call-sites.
> 
> I updated all forward declarers, and have uploaded separate CLs for each one.
> This CL will only be committed when all the others have landed. So only
> base/logging.h and base/string_piece.{h,cc} will be committed in this CL.
> 
> Here is a query that lists the related CLs:
> 
> http://goo.gl/GuheX
> 
> http://codereview.chromium.org/8659047/diff/1/base/string_piece.h
> File base/string_piece.h (right):
> 
> http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode36
> base/string_piece.h:36: namespace detail {
> On 2011/12/01 20:29:24, brettw wrote:
> > Normally this would be "internal" or something, but I don't actually think
> it's
> > necessary in this case. I think it's fine for this to just be in the base
> > namespace (just like basic_string is in std).
> 
> A compelling reason for basic_string to be in std is because you might
template
> a client class, method, or function on basic_string<CHAR>. The original CL
here
> did not really support that, since StringPiece was a subclass of
> BasicStringPiece while StringPiece16 was a direct instantiation.
> 
> I modified things so that both StringPiece and StringPiece16 are typedef'd to
> BasicStringPiece<std::string/string16>, which is itself a subclass of
> StringPieceDetail. The common behaviour is in StringPieceDetail while
> BasicStringPiece is specialized to add functionality to the single-byte
> versions.
> 
> With that in mind, I would propose that clients should never refer directly to
> StringPieceDetail, and it therefore belongs in an 'internal' namespace.
> 
> http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode50
> base/string_piece.h:50: typedef std::reverse_iterator<iterator>
> reverse_iterator;
> On 2011/12/01 20:29:24, brettw wrote:
> > I'd make this also const to match the non-reverse iterator (everything is a
> > const iterator in StringPiece).
> 
> In fact, why not just remove the non-const typedef's, since they are not used
> (if I follow the logical conclusions of your following comment).
> 
> http://codereview.chromium.org/8659047/diff/1/base/string_piece.h#newcode105
> base/string_piece.h:105: iterator begin() const { return ptr_; }
> On 2011/12/01 20:29:24, brettw wrote:
> > Can these return const_iterator instead? The return type will be the same,
but
> I
> > think it's best if everything matches.
> 
> Sure. Given this, I guess the next step would be to simply remove iterator and
> reverse_iterator from the definition, as they are no longer used?

I've done a similar job to the unittests, so now string_piece_unittest.cc will
also be committed in this CL.

[brettw, 2011-12-07 22:31:54]

Not being able to forward declare StringPiece in a reasonable way seems like a
very unfortunate regression. This is especially true since your patch makes this
header file more heavyweight to compile.

I liked your previous patch better where StringPiece was a class that derives
from the BasicStringPiece, but then would have made StringPiece16 also derive
(rather than a typedef) and just duplicate the constructors.

[erikwright (departed), 2011-12-12 18:17:31]

On 2011/12/07 22:31:54, brettw wrote:
> Not being able to forward declare StringPiece in a reasonable way seems like a
> very unfortunate regression. This is especially true since your patch makes
this
> header file more heavyweight to compile.
> 
> I liked your previous patch better where StringPiece was a class that derives
> from the BasicStringPiece, but then would have made StringPiece16 also derive
> (rather than a typedef) and just duplicate the constructors.

Hi Brett,

I think that's a very reasonable concern, but I have done an investigation that
shows there is no regression in practice.

There were a total of 6 headers that were previously appropriately
forward-declaring StringPiece. There were a few more that did so inappropriately
(i.e., they accepted StringPiece as a reference, and by not including
string_piece.h they prevent callers from trivially passing std::string/char*,
which is the point of the class).

It is only .o files that include these six files and do not otherwise include
string_piece.h that would see a compilation regression. I found exactly 4 .o
targets that match that description.

FWIW, I also did a benchmark for debug builds on a not-particularly fast Ubuntu
machine without distributed compilation and found the regression to be 1.9% real
time and 2.6%/3.9% user/kernel.

So there is no performance reason to choose one design over the other.

Furthermore, there are serious negative consequences to having two non-template
classes instead of a template. Primarily, code that is itself templated on char
type cannot directly instantiate an appropriate StringPiece class.

Therefore, I suggest that the current design is reasonable and appropriate.

Please let me know if you still have concerns about the basic approach.

Cheers,

Erik

[erikwright (departed), 2011-12-12 18:51:36]

I should clarify that, including test targets there are 12 that go from
forward-declaration only to full include of string_piece.h.

[brettw, 2011-12-12 18:59:52]

On 2011/12/12 18:17:31, erikwright wrote:
> Furthermore, there are serious negative consequences to having two
non-template
> classes instead of a template. Primarily, code that is itself templated on
char
> type cannot directly instantiate an appropriate StringPiece class.

I forgot about this use-case. I think this approach is fine. I'll do a last pass
on this code in a bit (gotta run to meeting now).

[brettw, 2011-12-12 20:19:28]

lgtm

[commit-bot: I haz the power, 2011-12-13 16:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/8659047/36001

[commit-bot: I haz the power, 2011-12-13 17:23:43]

Try job failure for 8659047-36001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[Nico, 2011-12-19 21:58:26]

http://codereview.chromium.org/8659047/diff/36001/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/8659047/diff/36001/base/string_piece.h#newcode239
base/string_piece.h:239: extern template class BasicStringPiece<std::string>;
hans@ points out that declaring specialized templates (like
"BasicStringPiece<std::string>", which is specialized above) is a no-op, since
the specialization causes the template to be instantiated already.

[erikwright (departed), 2011-12-20 15:26:41]

http://codereview.chromium.org/8659047/diff/36001/base/string_piece.h
File base/string_piece.h (right):

http://codereview.chromium.org/8659047/diff/36001/base/string_piece.h#newcode239
base/string_piece.h:239: extern template class BasicStringPiece<std::string>;
On 2011/12/19 21:58:26, Nico wrote:
> hans@ points out that declaring specialized templates (like
> "BasicStringPiece<std::string>", which is specialized above) is a no-op, since
> the specialization causes the template to be instantiated already.

Hmm. OK. The other concern would be the method definitions being duplicated in
many object files and not reconciled. But my intuition is that:

1) at worst only the methods used in a specific compilation unit would appear in
the object file, and most users only construct an instance before passing it
onto someone else.
2) even when methods are used, most of the methods amount to one or two
operations on a pointer that are probably cheaper than a function call anyway
and likely to be inlined.

So, thakis@ and hans@, would you say that it is harmless to simply remove the
externs here?

[Nico, 2011-12-20 20:54:11]

> Hmm. OK. The other concern would be the method definitions being duplicated in
> many object files and not reconciled. But my intuition is that:
> 
> 1) at worst only the methods used in a specific compilation unit would appear
in
> the object file, and most users only construct an instance before passing it
> onto someone else.

Hm, I thought compilers just emit everything, and leave it up to the linker to
sort out what's actually used.

> 2) even when methods are used, most of the methods amount to one or two
> operations on a pointer that are probably cheaper than a function call anyway
> and likely to be inlined.
> 
> So, thakis@ and hans@, would you say that it is harmless to simply remove the
> externs here?

The extern is a no-op (for BasicStringPiec<std::string> which has a
specialization; you should keep it for string16), so it doesn't matter if you
keep it or not.

Is the impact of this something you could just measure by building chrome with
and without this patch?

[erikwright (departed), 2011-12-20 21:16:00]

On 2011/12/20 20:54:11, Nico wrote:
> > Hmm. OK. The other concern would be the method definitions being duplicated
in
> > many object files and not reconciled. But my intuition is that:
> > 
> > 1) at worst only the methods used in a specific compilation unit would
appear
> in
> > the object file, and most users only construct an instance before passing it
> > onto someone else.
> 
> Hm, I thought compilers just emit everything, and leave it up to the linker to
> sort out what's actually used.

Yeah, I guess what I meant is that at worst only the methods used would be
retained during linking, and ideally, we expect them to be de-dup'd later if
appropriate.

> > 2) even when methods are used, most of the methods amount to one or two
> > operations on a pointer that are probably cheaper than a function call
anyway
> > and likely to be inlined.
> > 
> > So, thakis@ and hans@, would you say that it is harmless to simply remove
the
> > externs here?
> 
> The extern is a no-op (for BasicStringPiec<std::string> which has a
> specialization; you should keep it for string16), so it doesn't matter if you
> keep it or not.
> 
> Is the impact of this something you could just measure by building chrome with
> and without this patch?

Ultimately, since, as you say, it's a no-op, I guess it's irrelevant.

Thanks for the follow-up.

[erikwright (departed), 2011-12-22 02:29:23]

Hi Brett,

The win_shared bots did not like this patch, for reasons that are unclear to me
but seem to be caused by having a BASE_EXPORT specialization of a subclass,
while the parent class is a pure header-defined template.

Speculatively, I submitted a try job that made the std::string specialization
entirely header-defined, not BASE_EXPORT, and had its inline member methods
delegate to free functions that are BASE_EXPORT. That seems to have worked fine.

I wanted to give you a chance to respond, since this is a substantial departure
from the LGTM'd version. Alternatively to this, I could simply move the method
definitions to the header, but that seemed contradictory to your previous
requests.

For details on the breakage, see:

Build Failure:
http://build.chromium.org/p/tryserver.chromium/builders/win_shared/builds/791...

Template Definition:
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode43
Static Member Variable Definition:
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode145
Template Subclass Definition:
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode160
Template Subclass specialization:
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode180

[brettw, 2011-12-22 18:43:08]

I guess that seems OK. Do you think there will be more bloat as a
result or should everything cancel out?

Brett

On Wed, Dec 21, 2011 at 6:29 PM,  <erikwright@chromium.org> wrote:
> Hi Brett,
>
> The win_shared bots did not like this patch, for reasons that are unclear to
> me
> but seem to be caused by having a BASE_EXPORT specialization of a subclass,
> while the parent class is a pure header-defined template.
>
> Speculatively, I submitted a try job that made the std::string
> specialization
> entirely header-defined, not BASE_EXPORT, and had its inline member methods
> delegate to free functions that are BASE_EXPORT. That seems to have worked
> fine.
>
> I wanted to give you a chance to respond, since this is a substantial
> departure
> from the LGTM'd version. Alternatively to this, I could simply move the
> method
> definitions to the header, but that seemed contradictory to your previous
> requests.
>
> For details on the breakage, see:
>
> Build Failure:
>
http://build.chromium.org/p/tryserver.chromium/builders/win_shared/builds/791...
>
> Template Definition:
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode43
> Static Member Variable Definition:
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode145
> Template Subclass Definition:
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode160
> Template Subclass specialization:
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode180
>
>
> http://codereview.chromium.org/8659047/

[erikwright (departed), 2011-12-22 19:05:09]

My intuition is that it will be the same. I would expect methods of this
form to be inlined:

R m(const C& c, A a, B b);

class C {
 public:
  R m(A a, B b) const {
   return m(*this, a, b)
  }
};

I'll go ahead and press the commit button, but you can still veto between
now and X-knows-when it lands.

Thanks,

Erik

On Thu, Dec 22, 2011 at 1:43 PM, Brett Wilson <brettw@chromium.org> wrote:

> I guess that seems OK. Do you think there will be more bloat as a
> result or should everything cancel out?
>
> Brett
>
> On Wed, Dec 21, 2011 at 6:29 PM,  <erikwright@chromium.org> wrote:
> > Hi Brett,
> >
> > The win_shared bots did not like this patch, for reasons that are
> unclear to
> > me
> > but seem to be caused by having a BASE_EXPORT specialization of a
> subclass,
> > while the parent class is a pure header-defined template.
> >
> > Speculatively, I submitted a try job that made the std::string
> > specialization
> > entirely header-defined, not BASE_EXPORT, and had its inline member
> methods
> > delegate to free functions that are BASE_EXPORT. That seems to have
> worked
> > fine.
> >
> > I wanted to give you a chance to respond, since this is a substantial
> > departure
> > from the LGTM'd version. Alternatively to this, I could simply move the
> > method
> > definitions to the header, but that seemed contradictory to your previous
> > requests.
> >
> > For details on the breakage, see:
> >
> > Build Failure:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/win_shared/builds/791...
> >
> > Template Definition:
> >
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode43
> > Static Member Variable Definition:
> >
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode145
> > Template Subclass Definition:
> >
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode160
> > Template Subclass specialization:
> >
>
http://codereview.chromium.org/8659047/diff/43003/base/string_piece.h#newcode180
> >
> >
> > http://codereview.chromium.org/8659047/
>

[commit-bot: I haz the power, 2011-12-22 19:09:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/8659047/56001

[commit-bot: I haz the power, 2011-12-22 21:54:54]

Change committed as 115616