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 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.

(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

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

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 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.

(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/ )

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