Make gfx::Rect class operations consistently mutate the class they are called on.

Make gfx::Rect class operations consistently mutate the class they are called on.

Currently some methods mutate the class, and some return a new value, requiring
API users to know what kind of method they are calling each time, and making
inconsistent code. For example:

gfx::Rect rect;
rect.Inset(1, 1, 1, 1);
rect = rect.Intersect(other_rect);
rect.Offset(1, 1);

Instead of:

gfx::Rect rect;
rect.Inset(1, 1, 1, 1);
rect.Intersect(other_rect);
rect.Offset(1, 1);

We could go either way - making the class immutable or all methods return a new
instance - but I believe it is better to instead make all methods mutate the
class. This allows for shorter lines of code by avoiding having to repeat the
object's name twice in order to change it.

This patch changes gfx::Rect classes and all the callsites that uses these
methods. It should make no change in behaviour, so no new tests added.

R=sky
BUG=147395
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163579

[danakj, 2012-10-12 00:48:28]

The actual changes are in rect.h rect_f.h rect_base.h and rect_base_impl.h

The rest are just fluff to use the new methods.

[tfarina, 2012-10-12 01:37:27]

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base.h
File ui/gfx/rect_base.h (right):

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base.h#newcode125
ui/gfx/rect_base.h:125: // Become a rectangle that has the same center point but
with a size capped
nit: Becomes

[sky, 2012-10-12 16:26:37]

LGTM . this results in a bit more code, but it is less error prone.

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h
File ui/gfx/rect_base_impl.h (right):

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h#new...
ui/gfx/rect_base_impl.h:193: // special case empty rects...
Remove this comment, it depends clarify anything.

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h#new...
ui/gfx/rect_base_impl.h:195: SetRect(rect.x(), rect.y(), rect.width(),
rect.height());
this = rect ?

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (right):

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_f.h#newcode31
ui/gfx/rect_f.h:31: return Scale(scale, scale);
remove 'return' value.

[danakj, 2012-10-19 20:33:52]

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h
File ui/gfx/rect_base_impl.h (right):

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h#new...
ui/gfx/rect_base_impl.h:193: // special case empty rects...
On 2012/10/12 16:26:38, sky wrote:
> Remove this comment, it depends clarify anything.

Done.

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_base_impl.h#new...
ui/gfx/rect_base_impl.h:195: SetRect(rect.x(), rect.y(), rect.width(),
rect.height());
On 2012/10/12 16:26:38, sky wrote:
> this = rect ?

Done.

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (right):

http://codereview.chromium.org/11110004/diff/1047/ui/gfx/rect_f.h#newcode31
ui/gfx/rect_f.h:31: return Scale(scale, scale);
On 2012/10/12 16:26:38, sky wrote:
> remove 'return' value.

Done.

[danakj, 2012-10-19 22:12:57]

Looking for a bunch of OWNERS for this change.

Please take a look. There should be no change in behaviour, only to the Rect
api.

stevenjb: chrome/browser/notifications

thakis: ui/gfx/screen_mac.mm
thakis: content/browser/renderer_host/accelerated_surface_container_mac.cc
thakis: content/browser/renderer_host/backing_store_mac.mm
thakis: content/browser/renderer_host/render_widget_host_view_mac.mm
thakis: webkit/tools/test_shell/webwidget_host_mac.mm

piman: content/common/gpu
piman: content/plugin

jamesr: content/renderer
jamesr: webkit/tools

yzshen: ppapi

xiyuan: ui/app_list

sadrul: ui/views

jam: webkit/plugins

sky: ash/
sky: chrome/browser/ui
sky: chrome/browser/renderer_host
sky: ui/base
sky: ui/gfx

[danakj, 2012-10-19 22:13:17]

annnd.. i didnt cc owners.

[danakj, 2012-10-19 22:14:37]

Looking for a bunch of OWNERS for this change.

Please take a look. There should be no change in behaviour, only to the Rect
api.

stevenjb: chrome/browser/notifications

thakis: ui/gfx/screen_mac.mm
thakis: content/browser/renderer_host/accelerated_surface_container_mac.cc
thakis: content/browser/renderer_host/backing_store_mac.mm
thakis: content/browser/renderer_host/render_widget_host_view_mac.mm
thakis: webkit/tools/test_shell/webwidget_host_mac.mm

piman: content/common/gpu
piman: content/plugin

jamesr: content/renderer
jamesr: webkit/tools

yzshen: ppapi

xiyuan: ui/app_list

sadrul: ui/views

jam: webkit/plugins

sky: ash/
sky: chrome/browser/ui
sky: chrome/browser/renderer_host
sky: ui/base
sky: ui/gfx

[xiyuan, 2012-10-19 22:18:13]

ui/app_list LGTM

[stevenjb, 2012-10-19 22:48:11]

ash/system LGTM

[sadrul, 2012-10-19 23:15:06]

ui/views/ LGTM (note that sky@ covers this too)

[piman, 2012-10-20 00:03:05]

lgtm

[jam, 2012-10-20 02:33:35]

webkit/plugins lgtm

[Nico, 2012-10-20 03:14:43]

I'm not sure I buy the change description.

""" but I believe it is better to instead make all methods mutate the 
class. This allows for shorter lines of code by avoiding having to repeat the 
object's name twice in order to change it. """

Yet in the files I'm supposed to look at, the code got longer in most places,
and when I patch this in and run `git diff --stat branch`, I get:

 81 files changed, 367 insertions(+), 270 deletions(-)

Re """Currently some methods mutate the class, and some return a new value,
requiring API users to know what kind of method they are calling each time""":
Getting this wrong is caught by the compiler.

RectF currently has no mutating methods. RectBase has two mutating functions
(Inset() and Offset() and five functional-style methods. I guess it's nice to
make these methods consistent, but maybe the other way round?

[danakj, 2012-10-20 03:20:58]

On 2012/10/20 03:14:43, Nico wrote:
> I'm not sure I buy the change description.
> 
> """ but I believe it is better to instead make all methods mutate the 
> class. This allows for shorter lines of code by avoiding having to repeat the 
> object's name twice in order to change it. """
> 
> Yet in the files I'm supposed to look at, the code got longer in most places,
> and when I patch this in and run `git diff --stat branch`, I get:

Shorter lines, because you can do
descriptive_variable_name.Union(other_descriptive_variable_name);
on one line more often, instead of
descriptive_variable_name =
    descriptive_variable_name.Union(other_descriptive_variable_name);

>  81 files changed, 367 insertions(+), 270 deletions(-)
> 
> Re """Currently some methods mutate the class, and some return a new value,
> requiring API users to know what kind of method they are calling each time""":
> Getting this wrong is caught by the compiler.
> 
> RectF currently has no mutating methods. RectBase has two mutating functions
> (Inset() and Offset() and five functional-style methods. I guess it's nice to
> make these methods consistent, but maybe the other way round?

There are also set_x, set_width, set_position, etc. If we were going to make the
Rect objects completely immutable then sure, but I don't think we want to remove
setters, so I'd rather make the class consistent entirely.

[danakj, 2012-10-20 03:25:56]

On 2012/10/20 03:20:58, danakj wrote:
> On 2012/10/20 03:14:43, Nico wrote:
> > I'm not sure I buy the change description.
> > 
> > """ but I believe it is better to instead make all methods mutate the 
> > class. This allows for shorter lines of code by avoiding having to repeat
the 
> > object's name twice in order to change it. """
> > 
> > Yet in the files I'm supposed to look at, the code got longer in most
places,
> > and when I patch this in and run `git diff --stat branch`, I get:
>  >
> >  81 files changed, 367 insertions(+), 270 deletions(-)

I think your code is disproportionately affected with extra lines because almost
all the use of Rect methods are Scale. Thinks like Union/Intersect get shorter.
Scale seems to be less used in general, however than the others.

% git show|grep Scale|wc -l
56
% git show|grep Union|wc -l
112
% git show|grep Intersect|wc -l
130

[(unused - use chromium), 2012-10-20 03:30:13]

On Fri, Oct 19, 2012 at 8:20 PM,  <danakj@chromium.org> wrote:
> On 2012/10/20 03:14:43, Nico wrote:
>>
>> I'm not sure I buy the change description.
>
>
>> """ but I believe it is better to instead make all methods mutate the
>> class. This allows for shorter lines of code by avoiding having to repeat
>> the
>> object's name twice in order to change it. """
>
>
>> Yet in the files I'm supposed to look at, the code got longer in most
>> places,
>> and when I patch this in and run `git diff --stat branch`, I get:
>
>
> Shorter lines, because you can do
> descriptive_variable_name.Union(other_descriptive_variable_name);
> on one line more often, instead of
> descriptive_variable_name =
>     descriptive_variable_name.Union(other_descriptive_variable_name);

In my files, the result was more often that not assigned to a
different variable.

To check if that's just a sampling issue, I went through all files
below/ ash, and there too are 18 instances where the result of the
function call isn't assigned to the same variable, and just 5 where it
is.

>
>
>>   81 files changed, 367 insertions(+), 270 deletions(-)
>
>
>> Re """Currently some methods mutate the class, and some return a new
>> value,
>> requiring API users to know what kind of method they are calling each
>> time""":
>> Getting this wrong is caught by the compiler.
>
>
>> RectF currently has no mutating methods. RectBase has two mutating
>> functions
>> (Inset() and Offset() and five functional-style methods. I guess it's nice
>> to
>> make these methods consistent, but maybe the other way round?
>
>
> There are also set_x, set_width, set_position, etc. If we were going to make
> the
> Rect objects completely immutable then sure, but I don't think we want to
> remove
> setters, so I'd rather make the class consistent entirely.

set_ functions are a very common concept, I don't think there's
confusion about those not being "consistent" with the other functions.

[danakj, 2012-10-20 03:54:04]

On Sat, Oct 20, 2012 at 3:30 AM, Nico Weber <thakis@google.com> wrote:
> On Fri, Oct 19, 2012 at 8:20 PM,  <danakj@chromium.org> wrote:
>> On 2012/10/20 03:14:43, Nico wrote:
>>>
>>> I'm not sure I buy the change description.
>>
>>
>>> """ but I believe it is better to instead make all methods mutate the
>>> class. This allows for shorter lines of code by avoiding having to repeat
>>> the
>>> object's name twice in order to change it. """
>>
>>
>>> Yet in the files I'm supposed to look at, the code got longer in most
>>> places,
>>> and when I patch this in and run `git diff --stat branch`, I get:
>>
>>
>> Shorter lines, because you can do
>> descriptive_variable_name.Union(other_descriptive_variable_name);
>> on one line more often, instead of
>> descriptive_variable_name =
>>     descriptive_variable_name.Union(other_descriptive_variable_name);
>
> In my files, the result was more often that not assigned to a
> different variable.
>
> To check if that's just a sampling issue, I went through all files
> below/ ash, and there too are 18 instances where the result of the
> function call isn't assigned to the same variable, and just 5 where it
> is.

Well, if you consider the whole CL, then about half (56/116) of the
removed lines are of the form:

A = A.something() or A = B.something(A)

I don't know that metrics like these are really what matters so much
as clear code. Maybe putting multiple operations on a single line
makes for clearer code? I tend to think not though. I'll defer to
whatever sky@ likes, I just want to see consistency here, though I
obviously have a preference influenced by the cc/ code which was
written in this style with one operation per line.

>>
>>
>>>   81 files changed, 367 insertions(+), 270 deletions(-)
>>
>>
>>> Re """Currently some methods mutate the class, and some return a new
>>> value,
>>> requiring API users to know what kind of method they are calling each
>>> time""":
>>> Getting this wrong is caught by the compiler.
>>
>>
>>> RectF currently has no mutating methods. RectBase has two mutating
>>> functions
>>> (Inset() and Offset() and five functional-style methods. I guess it's nice
>>> to
>>> make these methods consistent, but maybe the other way round?
>>
>>
>> There are also set_x, set_width, set_position, etc. If we were going to make
>> the
>> Rect objects completely immutable then sure, but I don't think we want to
>> remove
>> setters, so I'd rather make the class consistent entirely.
>
> set_ functions are a very common concept, I don't think there's
> confusion about those not being "consistent" with the other functions.

[sky, 2012-10-22 15:18:22]

Here's my response earlier:

"LGTM . this results in a bit more code, but it is less error prone."

Yes, more code but seems more consistent and means we don't need the
WARN_UNUSED_RESULT.

[(unused - use chromium), 2012-10-22 15:27:43]

On Mon, Oct 22, 2012 at 8:18 AM,  <sky@chromium.org> wrote:
> Here's my response earlier:
>
> "LGTM . this results in a bit more code, but it is less error prone."
>
> Yes, more code but seems more consistent and means we don't need the
> WARN_UNUSED_RESULT.

I agree with "more consistent", but I disagree about "less error
prone" as the compiler catches this.

I agree that code like

    area = area.Subtract(right);

is more convoluted than just

    area.Subtract(right);

but I think this

    gfx::Rect wallpaper_cropped_rect = wallpaper_rect.Center(cropped_size);

reads a lot less stuttery than

    gfx::Rect wallpaper_cropped_rect = wallpaper_rect;
    wallpaper_cropped_rect.ClampToCenteredSize(cropped_size);

It's even worse in return statements, which now require a new
variable. This latter pattern seems more common, from looking through
the places touched in this CL.


But it's a fairly minor issue, and if Scott likes it, go ahead and
land. Personally I don't think this CL improves things.

[sreeram, 2012-10-22 15:44:30]

Why not have the best of both worlds?
class Rect {
  void Subtract(...) {...}
  Rect Subtract(...) const WARN_UNUSED_RESULT {...}
};

So per Nico's example, you could just do:
area.Subtract(right);
return wallpaper_rect.Subtract(cropped_size);

If you mix them up, the compiler will catch it:
area = area.Subtract(right);  // error: void value can't be assigned
wallpaper_rect.Subtract(cropped_size);  // error: return value being ignored

On Mon, Oct 22, 2012 at 8:27 AM, Nico Weber <thakis@google.com> wrote:
> On Mon, Oct 22, 2012 at 8:18 AM,  <sky@chromium.org> wrote:
>> Here's my response earlier:
>>
>> "LGTM . this results in a bit more code, but it is less error prone."
>>
>> Yes, more code but seems more consistent and means we don't need the
>> WARN_UNUSED_RESULT.
>
> I agree with "more consistent", but I disagree about "less error
> prone" as the compiler catches this.
>
> I agree that code like
>
>     area = area.Subtract(right);
>
> is more convoluted than just
>
>     area.Subtract(right);
>
> but I think this
>
>     gfx::Rect wallpaper_cropped_rect = wallpaper_rect.Center(cropped_size);
>
> reads a lot less stuttery than
>
>     gfx::Rect wallpaper_cropped_rect = wallpaper_rect;
>     wallpaper_cropped_rect.ClampToCenteredSize(cropped_size);
>
> It's even worse in return statements, which now require a new
> variable. This latter pattern seems more common, from looking through
> the places touched in this CL.
>
>
> But it's a fairly minor issue, and if Scott likes it, go ahead and
> land. Personally I don't think this CL improves things.

[danakj, 2012-10-22 15:47:55]

On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
<sreeram@chromium.org> wrote:
> Why not have the best of both worlds?
> class Rect {
>   void Subtract(...) {...}
>   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
> };

You can't overload on return type. You would have to name the methods
differently, which is making a more difficult-to-use API IMO.

[sreeram, 2012-10-22 15:49:10]

On 2012/10/22 15:47:55, danakj wrote:
> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
> <mailto:sreeram@chromium.org> wrote:
> > Why not have the best of both worlds?
> > class Rect {
> >   void Subtract(...) {...}
> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
> > };
> 
> You can't overload on return type. You would have to name the methods
> differently, which is making a more difficult-to-use API IMO.

I'm overloading on "const", not return type.

[danakj, 2012-10-22 15:57:25]

On Mon, Oct 22, 2012 at 11:49 AM,  <sreeram@chromium.org> wrote:
> On 2012/10/22 15:47:55, danakj wrote:
>>
>> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
>> <mailto:sreeram@chromium.org> wrote:
>> > Why not have the best of both worlds?
>> > class Rect {
>> >   void Subtract(...) {...}
>> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
>> > };
>
>
>> You can't overload on return type. You would have to name the methods
>> differently, which is making a more difficult-to-use API IMO.
>
>
> I'm overloading on "const", not return type.

I see, but that doesn't compile either. Compiler tries to use the void
function always. http://pastebin.com/YyWzLk0G

[sreeram, 2012-10-22 16:15:28]

On 2012/10/22 15:57:25, danakj wrote:
> On Mon, Oct 22, 2012 at 11:49 AM,  <mailto:sreeram@chromium.org> wrote:
> > On 2012/10/22 15:47:55, danakj wrote:
> >>
> >> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
> >> <mailto:sreeram@chromium.org> wrote:
> >> > Why not have the best of both worlds?
> >> > class Rect {
> >> >   void Subtract(...) {...}
> >> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
> >> > };
> >
> >
> >> You can't overload on return type. You would have to name the methods
> >> differently, which is making a more difficult-to-use API IMO.
> >
> >
> > I'm overloading on "const", not return type.
> 
> I see, but that doesn't compile either. Compiler tries to use the void
> function always. http://pastebin.com/YyWzLk0G

In your example ("return first.Subtract(second);"), |first| is a non-const
object, so the compiler will use the void method. In such a situation, you'll
have to write:
  return static_cast<const Int>(first).Subtract(second);

Admittedly, this is ugly. And we'd need good comments on top of the API methods
to let users know how to do this if they run into the "can't use void value"
error. However, my impression based on Nico's comments (disclaimer: I haven't
actually looked at all the call sites) is that most callers of the above sort
would have a const object, say something like:
  Int myfunc(const Int& first) {
    ...
    return first.Subtract(...);
  }
Which would work just fine without ugliness. Aka, the instances of ugliness
might be few and far between.

[sreeram, 2012-10-22 16:18:30]

On 2012/10/22 16:15:28, sreeram wrote:
>   return static_cast<const Int>(first).Subtract(second);

Self nit: Use const_cast<> instead.

[danakj, 2012-10-22 16:32:27]

On 2012/10/22 16:15:28, sreeram wrote:
> On 2012/10/22 15:57:25, danakj wrote:
> > On Mon, Oct 22, 2012 at 11:49 AM,  <mailto:sreeram@chromium.org> wrote:
> > > On 2012/10/22 15:47:55, danakj wrote:
> > >>
> > >> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
> > >> <mailto:sreeram@chromium.org> wrote:
> > >> > Why not have the best of both worlds?
> > >> > class Rect {
> > >> >   void Subtract(...) {...}
> > >> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
> > >> > };
> > >
> > >
> > >> You can't overload on return type. You would have to name the methods
> > >> differently, which is making a more difficult-to-use API IMO.
> > >
> > >
> > > I'm overloading on "const", not return type.
> > 
> > I see, but that doesn't compile either. Compiler tries to use the void
> > function always. http://pastebin.com/YyWzLk0G
> 
> In your example ("return first.Subtract(second);"), |first| is a non-const
> object, so the compiler will use the void method. In such a situation, you'll
> have to write:
>   return static_cast<const Int>(first).Subtract(second);
> 
> Admittedly, this is ugly. And we'd need good comments on top of the API
methods
> to let users know how to do this if they run into the "can't use void value"
> error. However, my impression based on Nico's comments (disclaimer: I haven't
> actually looked at all the call sites) is that most callers of the above sort
> would have a const object, say something like:
>   Int myfunc(const Int& first) {
>     ...
>     return first.Subtract(...);
>   }

That isn't the case, actually. The vast majority already have a local mutable
variable. I don't think forcing const_cast to make use of methods would be a
good thing for the API.

[(unused - use chromium), 2012-10-22 16:33:38]

On Mon, Oct 22, 2012 at 9:32 AM,  <danakj@chromium.org> wrote:
> On 2012/10/22 16:15:28, sreeram wrote:
>>
>> On 2012/10/22 15:57:25, danakj wrote:
>> > On Mon, Oct 22, 2012 at 11:49 AM,  <mailto:sreeram@chromium.org> wrote:
>> > > On 2012/10/22 15:47:55, danakj wrote:
>> > >>
>> > >> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
>> > >> <mailto:sreeram@chromium.org> wrote:
>> > >> > Why not have the best of both worlds?
>> > >> > class Rect {
>> > >> >   void Subtract(...) {...}
>> > >> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
>> > >> > };
>> > >
>> > >
>> > >> You can't overload on return type. You would have to name the methods
>> > >> differently, which is making a more difficult-to-use API IMO.
>> > >
>> > >
>> > > I'm overloading on "const", not return type.
>> >
>> > I see, but that doesn't compile either. Compiler tries to use the void
>> > function always. http://pastebin.com/YyWzLk0G
>
>
>> In your example ("return first.Subtract(second);"), |first| is a non-const
>> object, so the compiler will use the void method. In such a situation,
>> you'll
>> have to write:
>>    return static_cast<const Int>(first).Subtract(second);
>
>
>> Admittedly, this is ugly. And we'd need good comments on top of the API
>
> methods
>>
>> to let users know how to do this if they run into the "can't use void
>> value"
>> error. However, my impression based on Nico's comments (disclaimer: I
>> haven't
>> actually looked at all the call sites) is that most callers of the above
>> sort
>> would have a const object, say something like:
>>    Int myfunc(const Int& first) {
>>      ...
>>      return first.Subtract(...);
>>    }
>
>
> That isn't the case, actually. The vast majority already have a local
> mutable
> variable. I don't think forcing const_cast to make use of methods would be a
> good thing for the API.

I agree with this, fwiw :-)

[sreeram, 2012-10-22 16:43:03]

On 2012/10/22 16:33:38, thakis wrote:
> On Mon, Oct 22, 2012 at 9:32 AM,  <mailto:danakj@chromium.org> wrote:
> > On 2012/10/22 16:15:28, sreeram wrote:
> >>
> >> On 2012/10/22 15:57:25, danakj wrote:
> >> > On Mon, Oct 22, 2012 at 11:49 AM,  <mailto:sreeram@chromium.org> wrote:
> >> > > On 2012/10/22 15:47:55, danakj wrote:
> >> > >>
> >> > >> On Mon, Oct 22, 2012 at 11:43 AM, Sreeram Ramachandran
> >> > >> <mailto:sreeram@chromium.org> wrote:
> >> > >> > Why not have the best of both worlds?
> >> > >> > class Rect {
> >> > >> >   void Subtract(...) {...}
> >> > >> >   Rect Subtract(...) const WARN_UNUSED_RESULT {...}
> >> > >> > };
> >> > >
> >> > >
> >> > >> You can't overload on return type. You would have to name the methods
> >> > >> differently, which is making a more difficult-to-use API IMO.
> >> > >
> >> > >
> >> > > I'm overloading on "const", not return type.
> >> >
> >> > I see, but that doesn't compile either. Compiler tries to use the void
> >> > function always. http://pastebin.com/YyWzLk0G
> >
> >
> >> In your example ("return first.Subtract(second);"), |first| is a non-const
> >> object, so the compiler will use the void method. In such a situation,
> >> you'll
> >> have to write:
> >>    return static_cast<const Int>(first).Subtract(second);
> >
> >
> >> Admittedly, this is ugly. And we'd need good comments on top of the API
> >
> > methods
> >>
> >> to let users know how to do this if they run into the "can't use void
> >> value"
> >> error. However, my impression based on Nico's comments (disclaimer: I
> >> haven't
> >> actually looked at all the call sites) is that most callers of the above
> >> sort
> >> would have a const object, say something like:
> >>    Int myfunc(const Int& first) {
> >>      ...
> >>      return first.Subtract(...);
> >>    }
> >
> >
> > That isn't the case, actually. The vast majority already have a local
> > mutable
> > variable. I don't think forcing const_cast to make use of methods would be a
> > good thing for the API.
> 
> I agree with this, fwiw :-)

Actually, in the case of local (or class member) non-const objects, my
suggestion is identical to the original CL (which makes all methods mutating):

Previously:
  Rect area = ...;
  ...;
  return area.Subtract(...);

My suggestion or Dana's CL:
  Rect area = ...;
  ...;
  area.Subtract(...);
  return area;

[jamesr, 2012-10-22 17:47:35]

webkit/ lgtm

[commit-bot: I haz the power, 2012-10-23 00:49:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11110004/18102