Don't attempt to forward declare StringPiece.

Don't attempt to forward declare StringPiece.

1) This is discouraged because it prevents callers from benefiting from automatic coersion from string/char* types.
2) A follow-up CL (http://codereview.chromium.org/8659047/) will make StringPiece a template, and thus awkward to forward declare. The very small number of places that were appropriately forward declaring it do not justify writing a 'string_piece_forward.h'.

BUG=87634
R=cbentzel@chromium.org


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

[cbentzel, 2011-12-13 11:41:26]

It looks like you have forward declaration mechanism for this in logging.h -
although inlined in that header. 

Why not just add the header or duplicate that here?

[erikwright (departed), 2011-12-13 13:39:41]

I did an analysis that showed that for most files that were forward
declaring it, all targets that included those files were ultimately
including string_piece anyway, so there was no value in replicating a
verbose template and typedef forward declaration (or creating a
string_piece_foward.h header to do that).

logging.h is an extremely commonly used header, so the cost of unnecessary
includes is higher.

Also, unlike most other headers that declare classes returning StringPiece
instances (or accepting StringPiece pointers/collections), clients of
logging.h are not all that likely to actually use StringPiece. But, to be
honest, I have not measured the number of clients of logging.h that don't
already include string_piece.h.

You also make me realize that the code in logging.h that defines logging
support for StringPiece could just as easily be declared in string_piece.h,
which would then eliminate this inconsistency. I'll go ahead and do that
before committing the follow-up CL.

Let me know if you have any other questions.

Thanks,

Erik

On Dec 13, 2011 6:41 AM, <cbentzel@chromium.org> wrote:

> It looks like you have forward declaration mechanism for this in logging.h
> -
> although inlined in that header.
>
> Why not just add the header or duplicate that here?
>
>
http://codereview.chromium.**org/8929002/<http://codereview.chromium.org/8929...
>

[erikwright (departed), 2011-12-13 13:54:06]

Actually, on second thought, moving the logging support from logging.h to
string_piece.h implies including <ostream> there, which is undesirable.

So I'm going to leave that declaration in logging.h.

I went ahead and did a count of .o targets that include logging.h but not
string_piece.h. It's 1347 targets. This is 100 times more targets than all of
the other headers that I converted from forward-decls to includes (12 targets).
Therefore I think the decision to forward declare in logging.h and nowhere else
is appropriate.

On 2011/12/13 13:39:41, erikwright wrote:
> I did an analysis that showed that for most files that were forward
> declaring it, all targets that included those files were ultimately
> including string_piece anyway, so there was no value in replicating a
> verbose template and typedef forward declaration (or creating a
> string_piece_foward.h header to do that).
> 
> logging.h is an extremely commonly used header, so the cost of unnecessary
> includes is higher.
> 
> Also, unlike most other headers that declare classes returning StringPiece
> instances (or accepting StringPiece pointers/collections), clients of
> logging.h are not all that likely to actually use StringPiece. But, to be
> honest, I have not measured the number of clients of logging.h that don't
> already include string_piece.h.
> 
> You also make me realize that the code in logging.h that defines logging
> support for StringPiece could just as easily be declared in string_piece.h,
> which would then eliminate this inconsistency. I'll go ahead and do that
> before committing the follow-up CL.
> 
> Let me know if you have any other questions.
> 
> Thanks,
> 
> Erik
> 
> On Dec 13, 2011 6:41 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=cbentzel@chromium.org>
wrote:
> 
> > It looks like you have forward declaration mechanism for this in logging.h
> > -
> > although inlined in that header.
> >
> > Why not just add the header or duplicate that here?
> >
> >
>
http://codereview.chromium.**org/8929002/%3Chttp://codereview.chromium.org/89...>
> >

[cbentzel, 2011-12-13 14:50:15]

LGTM

thanks for the analysis and explanation

On Tue, Dec 13, 2011 at 8:54 AM, <erikwright@chromium.org> wrote:

> Actually, on second thought, moving the logging support from logging.h to
> string_piece.h implies including <ostream> there, which is undesirable.
>
> So I'm going to leave that declaration in logging.h.
>
> I went ahead and did a count of .o targets that include logging.h but not
> string_piece.h. It's 1347 targets. This is 100 times more targets than all
> of
> the other headers that I converted from forward-decls to includes (12
> targets).
> Therefore I think the decision to forward declare in logging.h and nowhere
> else
> is appropriate.
>
>
> On 2011/12/13 13:39:41, erikwright wrote:
>
>> I did an analysis that showed that for most files that were forward
>> declaring it, all targets that included those files were ultimately
>> including string_piece anyway, so there was no value in replicating a
>> verbose template and typedef forward declaration (or creating a
>> string_piece_foward.h header to do that).
>>
>
>  logging.h is an extremely commonly used header, so the cost of unnecessary
>> includes is higher.
>>
>
>  Also, unlike most other headers that declare classes returning StringPiece
>> instances (or accepting StringPiece pointers/collections), clients of
>> logging.h are not all that likely to actually use StringPiece. But, to be
>> honest, I have not measured the number of clients of logging.h that don't
>> already include string_piece.h.
>>
>
>  You also make me realize that the code in logging.h that defines logging
>> support for StringPiece could just as easily be declared in
>> string_piece.h,
>> which would then eliminate this inconsistency. I'll go ahead and do that
>> before committing the follow-up CL.
>>
>
>  Let me know if you have any other questions.
>>
>
>  Thanks,
>>
>
>  Erik
>>
>
>  On Dec 13, 2011 6:41 AM,
>>
> <https://mail.google.com/mail/**?view=cm&fs=1&tf=1&to=**
>
cbentzel@chromium.org<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=cbentzel@chromium.org>
> >
>
> wrote:
>
>  > It looks like you have forward declaration mechanism for this in
>> logging.h
>> > -
>> > although inlined in that header.
>> >
>> > Why not just add the header or duplicate that here?
>> >
>> >
>>
>
> http://codereview.chromium.****org/8929002/%3Chttp://coderevi**
> ew.chromium.org/8929002/ <http://codereview.chromium.org/8929002/>>
>
>> >
>>
>
>
>
http://codereview.chromium.**org/8929002/<http://codereview.chromium.org/8929...
>

[commit-bot: I haz the power, 2011-12-13 14:54:43]

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

[commit-bot: I haz the power, 2011-12-13 16:27:09]

Change committed as 114219