Remove all uses of EmptyString16(), EmptyWString(), and EmptyGURL(), and thei...

Remove most uses of EmptyString(), EmptyWString(), EmptyString16(), and EmptyGURL(), since the code in question can just use the default constructor.

BUG=none
TEST=none

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

[brettw, 2010-01-07 06:21:43]

http://codereview.chromium.org/517054/diff/4001/4025
File chrome/browser/autofill/form_group.h (right):

http://codereview.chromium.org/517054/diff/4001/4025#newcode11
chrome/browser/autofill/form_group.h:11: #include "base/string_util.h"
I bet you can now remove this include.

http://codereview.chromium.org/517054/diff/4001/4040
File chrome/browser/tab_contents/tab_contents.h (left):

http://codereview.chromium.org/517054/diff/4001/4040#oldcode194
chrome/browser/tab_contents/tab_contents.h:194: virtual const GURL& GetURL()
const;
I kind of liked having these avoid a copy, since they are called all the time.
You could argue whether its worth it to do, but removing it seems undesirable to
me.

[Peter Kasting, 2010-01-07 06:58:33]

http://codereview.chromium.org/517054/diff/4001/4040
File chrome/browser/tab_contents/tab_contents.h (left):

http://codereview.chromium.org/517054/diff/4001/4040#oldcode194
chrome/browser/tab_contents/tab_contents.h:194: virtual const GURL& GetURL()
const;
On 2010/01/07 06:21:44, brettw wrote:
> I kind of liked having these avoid a copy, since they are called all the time.
> You could argue whether its worth it to do, but removing it seems undesirable
to
> me. 

But you have long told me to treat GURL like std::string, and we agreed from
early on in the project that returning strings by value was just as performant
as by const ref (and you were the one that did the tests to prove it!).  In fact
you even explicitly said at one point later that returning GURLs by value is
fine.  And so we do it all over the place.  Why is this case suddenly different
than the whole rest of the codebase?

[darin (slow to review), 2010-01-07 07:55:03]

On Wed, Jan 6, 2010 at 10:58 PM, <pkasting@chromium.org> wrote:

>
> http://codereview.chromium.org/517054/diff/4001/4040
> File chrome/browser/tab_contents/tab_contents.h (left):
>
> http://codereview.chromium.org/517054/diff/4001/4040#oldcode194
> chrome/browser/tab_contents/tab_contents.h:194: virtual const GURL&
> GetURL() const;
> On 2010/01/07 06:21:44, brettw wrote:
>
>> I kind of liked having these avoid a copy, since they are called all
>>
> the time.
>
>> You could argue whether its worth it to do, but removing it seems
>>
> undesirable to
>
>> me.
>>
>
> But you have long told me to treat GURL like std::string, and we agreed
> from early on in the project that returning strings by value was just as
> performant as by const ref (and you were the one that did the tests to
> prove it!).  In fact you even explicitly said at one point later that
> returning GURLs by value is fine.  And so we do it all over the place.
> Why is this case suddenly different than the whole rest of the codebase?
>
>
I'm also unhappy with this change.  It is good practice to use "const Foo&"
for the return value of a property getter.  This allows the property
accessor to be as cheap as a direct member variable access.  Return by value
is essential if you want to be able to dynamically generate the return
value.  In some cases it is rare that you wouldn't have a member variable to
return, so static empty strings and GURLs are quite handy.

-Darin



>
> http://codereview.chromium.org/517054
>

[darin (slow to review), 2010-01-07 07:57:11]

To clarify, I think that much of this CL is really great.  It should be quite
rare that EmptyString is needed, and yet as this CL indicates, it is overused. 
EmptyString() should only be used where std::string() cannot be used.

[Peter Kasting, 2010-01-07 08:05:34]

On 2010/01/07 07:55:03, darin wrote:
> I'm also unhappy with this change.  It is good practice to use "const Foo&"
> for the return value of a property getter.  This allows the property
> accessor to be as cheap as a direct member variable access.

My comment is the same as with Brett's: We've been _extremely_ clear, for
several years now, that for strings and GURLs, engineers should not worry about
the cost of return by const ref vs. return by value, because in practically all
cases the compiler optimizes them to the same thing, and people should have no
qualms about returning things by value.

Your comment flies in the face of this longstanding, oft-reconfirmed coding
pattern (of which we have many, many thousands of cases in our codebase today),
and given Brett's past demonstrations with generated assembly, I simply don't
buy it.  I am convinced this change won't affect perf and will be quite happy to
watch the bots to make sure.

Not only does EmptyGURL() give no benefit for this case, removing it allows us
to eliminate that function entirely, eliminating all possibility that people
will misuse it, as many were before this change.  EmptyXXX() is inherently
confusing and it would be worth paying some costs to eliminate it; luckily, we
don't need to pay any.

[Peter Kasting, 2010-01-07 08:48:57]

On Thu, Jan 7, 2010 at 12:05 AM, <pkasting@chromium.org> wrote:

> We've been _extremely_ clear, for
> several years now, that for strings and GURLs, engineers should not worry
> about
> the cost of return by const ref vs. return by value, because in practically
> all
> cases the compiler optimizes them to the same thing, and people should have
> no
> qualms about returning things by value.
>

I still say this is the advice we've given, and followed (in much of the
codebase), but Darin pointed out to me that the distinction is between when
the callee has to generate a string (even one it will be copying to a member
that it then returns), in which case the Return Value Optimization lets the
compiler generate it straight into the return object, and when it's a pure
accessor of an existing member string, in which case the compiler has to be
noticeably smarter.  He sent me a testcase GCC doesn't do well on, tomorrow
we'll see if MSVC does better.  If not, the question is how we can find, and
safely fix, the existing code -- it's enormous, and you can't blindly
replace return-by-value with return-by-const-ref even in simple functions,
depending on what they and their callers do.

It's not obvious, even if the two do not generate identical code, that there
will be a noticeable perf effect -- and the benefits of completely
eliminating these sorts of functions are nonzero.  So I'd still like to try
doing this even in that case, and even if I have to split the patch into
controversial and noncontroversial pieces.

PK


> http://codereview.chromium.org/517054
>

[darin (slow to review), 2010-01-07 08:53:18]

To be clear, there was a significant code size difference between return by
reference and return by value for std::string.  If it is true that we have
overused return by value, then we could see some benefits here.  At least,
we should try to make sure we do the right thing for new code.

-Darin


On Thu, Jan 7, 2010 at 12:48 AM, Peter Kasting <pkasting@chromium.org>wrote:

> On Thu, Jan 7, 2010 at 12:05 AM, <pkasting@chromium.org> wrote:
>
> We've been _extremely_ clear, for
>> several years now, that for strings and GURLs, engineers should not worry
>> about
>> the cost of return by const ref vs. return by value, because in
>> practically all
>> cases the compiler optimizes them to the same thing, and people should
>> have no
>> qualms about returning things by value.
>>
>
> I still say this is the advice we've given, and followed (in much of the
> codebase), but Darin pointed out to me that the distinction is between when
> the callee has to generate a string (even one it will be copying to a member
> that it then returns), in which case the Return Value Optimization lets the
> compiler generate it straight into the return object, and when it's a pure
> accessor of an existing member string, in which case the compiler has to be
> noticeably smarter.  He sent me a testcase GCC doesn't do well on, tomorrow
> we'll see if MSVC does better.  If not, the question is how we can find, and
> safely fix, the existing code -- it's enormous, and you can't blindly
> replace return-by-value with return-by-const-ref even in simple functions,
> depending on what they and their callers do.
>
> It's not obvious, even if the two do not generate identical code, that
> there will be a noticeable perf effect -- and the benefits of completely
> eliminating these sorts of functions are nonzero.  So I'd still like to try
> doing this even in that case, and even if I have to split the patch into
> controversial and noncontroversial pieces.
>
> PK
>
>
>> http://codereview.chromium.org/517054
>>
>
>

[brettw, 2010-01-07 19:27:11]

On Thu, Jan 7, 2010 at 6:58 AM,  <pkasting@chromium.org> wrote:
>
> http://codereview.chromium.org/517054/diff/4001/4040
> File chrome/browser/tab_contents/tab_contents.h (left):
>
> http://codereview.chromium.org/517054/diff/4001/4040#oldcode194
> chrome/browser/tab_contents/tab_contents.h:194: virtual const GURL&
> GetURL() const;
> On 2010/01/07 06:21:44, brettw wrote:
>>
>> I kind of liked having these avoid a copy, since they are called all
>
> the time.
>>
>> You could argue whether its worth it to do, but removing it seems
>
> undesirable to
>>
>> me.
>
> But you have long told me to treat GURL like std::string, and we agreed
> from early on in the project that returning strings by value was just as
> performant as by const ref (and you were the one that did the tests to
> prove it!).  In fact you even explicitly said at one point later that
> returning GURLs by value is fine.  And so we do it all over the place.
> Why is this case suddenly different than the whole rest of the codebase?

You're misremembering. Returning by value is totally different than
returning a const ref. The things I compared is returning by value
versus returning by out param. Having an out param is the same as what
you did in your patch, which is one copy. The current code has 0.

Brett

[Peter Kasting, 2010-01-07 21:18:49]

I believe I have removed all the cases where I changed return-by-const-ref to
return-by-value.  Let me know if you see any.

[darin (slow to review), 2010-01-07 21:34:08]

LGTM, except i think i spotted a bug in the existing code...

http://codereview.chromium.org/517054/diff/2026/3101
File chrome/browser/autofill/address.cc (right):

http://codereview.chromium.org/517054/diff/2026/3101#newcode69
chrome/browser/autofill/address.cc:69: string16 Address::GetFieldText(const
AutoFillType& type) const {
this is a great example of a method that should return |const string16&|.  all
of the submethods, line1, line2, etc. should also be changed to return |const
string16&|.  doing so would reduce code bloat.  this can be done as a separate
CL.

http://codereview.chromium.org/517054/diff/2026/3098
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/517054/diff/2026/3098#newcode87
chrome/browser/autofill/contact_info.cc:87: MiddleInitial();
was this supposed to have a return statement on it?  looks like a bug to me.

[Peter Kasting, 2010-01-07 22:10:37]

http://codereview.chromium.org/517054/diff/2026/3101
File chrome/browser/autofill/address.cc (right):

http://codereview.chromium.org/517054/diff/2026/3101#newcode69
chrome/browser/autofill/address.cc:69: string16 Address::GetFieldText(const
AutoFillType& type) const {
On 2010/01/07 21:34:08, darin wrote:
> this is a great example of a method that should return |const string16&|.  all
> of the submethods, line1, line2, etc. should also be changed to return |const
> string16&|.  doing so would reduce code bloat.  this can be done as a separate
> CL.

Filed a bug against jhawkins since lots of autofill code looks like it uses
these patterns.

http://codereview.chromium.org/517054/diff/2026/3098
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/517054/diff/2026/3098#newcode87
chrome/browser/autofill/contact_info.cc:87: MiddleInitial();
On 2010/01/07 21:34:08, darin wrote:
> was this supposed to have a return statement on it?  looks like a bug to me.

I bet it was.  I added one.