Closed Bug 1002289 Opened 6 years ago Closed 6 years ago

Allow adding/subtracting of BaseSize on BaseRect

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: drs, Assigned: drs)

Details

Attachments

(1 file, 1 obsolete file)

Currently, we don't allow adding or subtracting a BaseSize to BaseRect with a +/-/+=/-= operators. While this may prevent type errors, I think it's more likely that we want to add/subtract the width and height dimensions of the BaseSize to the BaseRect's.
Assignee: nobody → drs+bugzilla
Status: NEW → ASSIGNED
Attachment #8413471 - Flags: review?(botond)
Comment on attachment 8413471 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Review of attachment 8413471 [details] [diff] [review]:
-----------------------------------------------------------------

Let's also flag Kats to see if he agrees that the operator being added here is reasonable from a class interface point of view.

::: gfx/2d/BaseRect.h
@@ +284,5 @@
> +  }
> +  Sub operator-(const SizeT& aSize) const
> +  {
> +    return Sub(x, y, width - aSize.width, height - aSize.height);
> +  }

There is an idiom in C++ for reusing code between the "a op b" and the "a op= b" forms of an overloaded operator by implementing the former in terms of the latter (see http://herbsutter.com/2013/05/20/gotw-4-class-mechanics/, point 4).

I realize that other operators in this class don't use it, but I think it's better to start late than never.
Attachment #8413471 - Flags: review?(bugmail.mozilla)
Comment on attachment 8413471 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Review of attachment 8413471 [details] [diff] [review]:
-----------------------------------------------------------------

The interface changes seem ok to me but somebody who owns this code should review it.
Attachment #8413471 - Flags: review?(bugmail.mozilla) → review?(bas)
Comment on attachment 8413471 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Review of attachment 8413471 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/BaseRect.h
@@ +284,5 @@
> +  }
> +  Sub operator-(const SizeT& aSize) const
> +  {
> +    return Sub(x, y, width - aSize.width, height - aSize.height);
> +  }

I agree.
Attachment #8413471 - Flags: review?(bas) → review+
(In reply to Botond Ballo [:botond] from comment #2)
> There is an idiom in C++ for reusing code between the "a op b" and the "a
> op= b" forms of an overloaded operator by implementing the former in terms
> of the latter (see http://herbsutter.com/2013/05/20/gotw-4-class-mechanics/,
> point 4).
> 
> I realize that other operators in this class don't use it, but I think it's
> better to start late than never.

I'm not sure how to do this. We need to take the operator+ out of the BaseRect class to do this, but it's templated so I can't refer to it entirely generically. Maybe my C++-fu is failing me here.
(In reply to Doug Sherk (:drs) from comment #5)
> (In reply to Botond Ballo [:botond] from comment #2)
> > There is an idiom in C++ for reusing code between the "a op b" and the "a
> > op= b" forms of an overloaded operator by implementing the former in terms
> > of the latter (see http://herbsutter.com/2013/05/20/gotw-4-class-mechanics/,
> > point 4).
> > 
> > I realize that other operators in this class don't use it, but I think it's
> > better to start late than never.
> 
> I'm not sure how to do this. We need to take the operator+ out of the
> BaseRect class to do this, but it's templated so I can't refer to it
> entirely generically. Maybe my C++-fu is failing me here.

Hmm, good point. I think we can dispense with the "pass the first parameter by value" optimization and write operator+ as follows:

    Sub operator+(const SizeT& aSize) const
    {
        Sub result(static_cast<const Sub&>(*this));
        result += aSize;
        return result;
    }
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Doug Sherk (:drs) from comment #5)
> > (In reply to Botond Ballo [:botond] from comment #2)
> > > There is an idiom in C++ for reusing code between the "a op b" and the "a
> > > op= b" forms of an overloaded operator by implementing the former in terms
> > > of the latter (see http://herbsutter.com/2013/05/20/gotw-4-class-mechanics/,
> > > point 4).
> > > 
> > > I realize that other operators in this class don't use it, but I think it's
> > > better to start late than never.
> > 
> > I'm not sure how to do this. We need to take the operator+ out of the
> > BaseRect class to do this, but it's templated so I can't refer to it
> > entirely generically. Maybe my C++-fu is failing me here.
> 
> Hmm, good point. I think we can dispense with the "pass the first parameter
> by value" optimization and write operator+ as follows:
> 
>     Sub operator+(const SizeT& aSize) const
>     {
>         Sub result(static_cast<const Sub&>(*this));
>         result += aSize;
>         return result;
>     }

This problem made me curious enough to ask about this on StackOverflow [1].

It seems we can use an in-class 'friend' declaration FTW:

    friend Sub operator+(Sub aRect, const SizeT& aSize)
    {
        return aRect += aSize;
    }

[1] http://stackoverflow.com/questions/23377405/how-can-i-implement-op-in-terms-of-op-in-a-crtp-base-class
Comment on attachment 8413471 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Review of attachment 8413471 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag until patch is revised as per comment 2 (see comment 7 for guidance).
Attachment #8413471 - Flags: review?(botond)
Cool, that worked! I decided to fix the operators for Rect+/-Point, too.
Attachment #8413471 - Attachment is obsolete: true
Attachment #8416540 - Flags: review?(botond)
Comment on attachment 8416540 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Review of attachment 8416540 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8416540 - Flags: review?(botond) → review+
Comment on attachment 8416540 [details] [diff] [review]
Allow adding/subtracting of BaseSize on BaseRect.

Carrying r+ from Bas.
Attachment #8416540 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/db686f9cc96f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Sorry, I'm used to pushing to Gaia these days. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/db686f9cc96f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.