1016757 - Add an overload of TimeDuration::operator* that takes uint_64t

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

As suggested in bug 1007513 comment 2, what about allowing uint64_t as a multiplier? As pointed out in that comment, that will lead to wrapping with large uint64_t but that is already the case for large int64_t.

Comment 1

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

Attachment: Add TimeDuration::operator* (const uint64_t) const, i.e. accept an unsigned 64-bit integer as a multiplier

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8429733 [details] [diff] [review]
Add TimeDuration::operator* (const uint64_t) const, i.e. accept an unsigned 64-bit integer as a multiplier

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

Asking dholbert for f?, since he suggested the method, but I wasn't clear on his rationale for putting the method in TimeDuration rather than just casting to int64_t at the callsite.

::: xpcom/ds/TimeStamp.h
@@ +112,5 @@
>    TimeDuration operator*(const uint32_t aMultiplier) const {
>      return TimeDuration::FromTicks(mValue * int64_t(aMultiplier));
>    }
>    TimeDuration operator*(const int64_t aMultiplier) const {
> +    return TimeDuration::FromTicks(mValue * aMultiplier);

Thanks for removing the no-op cast here.

@@ +119,1 @@
>      return TimeDuration::FromTicks(mValue * int64_t(aMultiplier));

I think it'd be better if callers wanting to multiply by uint64_t were forced to do the conversion to int64_t themselves, so there was at least some small quantity of thought devoted to the case of values > INT64_MAX.

Does the right thing happen for the animation case when aMultiplier is > INT64_MAX?  I realize that having an animation with 9 quintillion iterations is...unlikely, but I am still curious.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Asking dholbert for f?, since he suggested the method, but I wasn't clear on
> his rationale for putting the method in TimeDuration rather than just
> casting to int64_t at the callsite.
[...]
> I think it'd be better if callers wanting to multiply by uint64_t were
> forced to do the conversion to int64_t themselves, so there was at least
> some small quantity of thought devoted to the case of values > INT64_MAX.

Hmm.  I somewhat-agree.

However, the question is, will developers who want to pass in uint64_t values *actually know* that the right thing to do is cast to int64_t?

e.g. people may (incorrectly) think that it's more appropriate to cast to uint32_t instead (preserving the 'unsigned-ness' of the parameter), and lose the ability to pass in values > UINT32_MAX.  (This is what we initially had, in the patch on bug 1007513.)

If we don't add a new operator*(), I'd think we should at *least* add a comment to TimeDuration, explaining what the recommended course of action is in this case (cast to int64_t, and think about the consequences).

> Does the right thing happen for the animation case when aMultiplier is >
> INT64_MAX?

Hmm.  I thought so, but now I'm not sure.

Based on the question's prose in [1], it sounds like conversions from signed to unsigned are well-defined, **but the reverse is not true**, when converting unsigned values over INT64_MAX into a signed 64-bit int.  It's compiler-impl-defined.  So, that probably makes any assumptions a bit evil.  (Note that this would also make any *caller's* static_casts<int64_t> similarly-evil.)

So perhaps we should write a non-evil operator*() impl which explicitly checks whether the input is <= INT64_MAX, and in that case, static_cast the input to int64_t; and for larger inputs, we could throw up our hands & set the duration to INT64_MAX? (perhaps with a NS_WARNING)

[1] http://stackoverflow.com/questions/13150449/efficient-unsigned-to-signed-cast-avoiding-implementation-defined-behavior

Comment 4

[Daniel Holbert [:dholbert]]

[needinfo=froydnj on the proposal at the end of comment 3]

Comment 5

[Nathan Froyd [:froydnj]]

I think truncating overly-large uint64_t values to INT64_MAX is as reasonable as anything.  Things still go "wrong", but at least it's a well-defined sort of wrongness, rather than suddenly swapping signs of things unexpectedly.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8429733 [details] [diff] [review]
Add TimeDuration::operator* (const uint64_t) const, i.e. accept an unsigned 64-bit integer as a multiplier

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

r=me with the truncation scheme from comment 3 and comment 5 implemented.  Your choice whether to add a dash of NS_WARNING/MOZ_ASSERT.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Things still go "wrong", but at least it's a
> well-defined sort of wrongness, rather than suddenly swapping signs of
> things unexpectedly.

Agreed. (And the wrongness from signed-conversion could actually be worse than swapping signs -- technically, the compiler could make us launch nethack or do anything it likes, since this would be implementation-defined behavior.  See http://feross.org/gcc-ownage/ )

Comment 8

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

Attachment: Add TimeDuration::operator* (const uint64_t) const, i.e. accept an unsigned 64-bit integer as a multiplier;

Currently running this through try: https://tbpl.mozilla.org/?tree=Try&rev=1198dc3fbdfe

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b22ee006583

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b22ee006583