Closed
Bug 1002289
Opened 10 years ago
Closed 10 years ago
Allow adding/subtracting of BaseSize on BaseRect
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: drs, Assigned: drs)
Details
Attachments
(1 file, 1 obsolete file)
2.04 KB,
patch
|
botond
:
review+
drs
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
(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; }
Comment 7•10 years ago
|
||
(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 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8416540 [details] [diff] [review] Allow adding/subtracting of BaseSize on BaseRect. Carrying r+ from Bas.
Attachment #8416540 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db686f9cc96f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•10 years ago
|
||
Sorry, I'm used to pushing to Gaia these days. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db686f9cc96f
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•