1203350 - Unify certain arithmetic operators in TimeStamp

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Follow-up from bug 1202556, we should remove duplicate code between operator+ and operator+= by writing one in terms of the other. Likewise for subtraction.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Unify arithmetic operators in TimeStamp

Comment 2

[Nathan Froyd [:froydnj]]

Thank you!

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8658995 [details] [diff] [review]
Unify arithmetic operators in TimeStamp

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

One minor thing I thought of after the fact:

::: mozglue/misc/TimeStamp.h
@@ +491,5 @@
>  
>    TimeStamp operator+(const TimeDuration& aOther) const
>    {
> +    TimeStamp result = *this;
> +    return result += aOther;

Actually, can you write this as:

result += aOther;
return result;

so the compiler's named-value return optimization kicks in?  I'm unsure of whether the right thing would happen with trying to copy-constructor the returned reference from operator+=.

@@ +496,5 @@
>    }
>    TimeStamp operator-(const TimeDuration& aOther) const
>    {
> +    TimeStamp result = *this;
> +    return result -= aOther;

Likewise here.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Actually, can you write this as:
> 
> result += aOther;
> return result;
> 
> so the compiler's named-value return optimization kicks in?  I'm unsure of
> whether the right thing would happen with trying to copy-constructor the
> returned reference from operator+=.

Yeah, I was wondering the exact same thing. I did some digging around to see if compiler's would apply RVO if I rolled up the two statements and I got the impression from MSVC's docs that it probably would (it only called out returning separately-named objects as problematic) but better to be safe.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d47f9818ce

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2d47f9818ce