Allow constexpr variables of TimeDelta type.

base/time/time.h

Index: base/time/time.h
diff --git a/base/time/time.h b/base/time/time.h
index 9fce71b581e4ca27c216e31cb40ab78b867760ea..4e12b532016f3b73c78efe1ca17bc5aeca3e1cfe 100644
--- a/base/time/time.h
+++ b/base/time/time.h
@@ -106,14 +106,14 @@ class BASE_EXPORT TimeDelta {
   }
 
   // Converts units of time to TimeDeltas.
-  static TimeDelta FromDays(int days);
-  static TimeDelta FromHours(int hours);
-  static TimeDelta FromMinutes(int minutes);
-  static TimeDelta FromSeconds(int64_t secs);
-  static TimeDelta FromMilliseconds(int64_t ms);
-  static TimeDelta FromSecondsD(double secs);
-  static TimeDelta FromMillisecondsD(double ms);
-  static TimeDelta FromMicroseconds(int64_t us);
+  static constexpr TimeDelta FromDays(int days);
+  static constexpr TimeDelta FromHours(int hours);
+  static constexpr TimeDelta FromMinutes(int minutes);
+  static constexpr TimeDelta FromSeconds(int64_t secs);
+  static constexpr TimeDelta FromMilliseconds(int64_t ms);
+  static constexpr TimeDelta FromSecondsD(double secs);
+  static constexpr TimeDelta FromMillisecondsD(double ms);
+  static constexpr TimeDelta FromMicroseconds(int64_t us);
 #if defined(OS_WIN)
   static TimeDelta FromQPCValue(LONGLONG qpc_value);
 #endif
@@ -222,22 +222,22 @@ class BASE_EXPORT TimeDelta {
   }
 
   // Comparison operators.
-  bool operator==(TimeDelta other) const {
+  constexpr bool operator==(TimeDelta other) const {
     return delta_ == other.delta_;
   }
-  bool operator!=(TimeDelta other) const {
+  constexpr bool operator!=(TimeDelta other) const {
     return delta_ != other.delta_;
   }
-  bool operator<(TimeDelta other) const {
+  constexpr bool operator<(TimeDelta other) const {
     return delta_ < other.delta_;
   }
-  bool operator<=(TimeDelta other) const {
+  constexpr bool operator<=(TimeDelta other) const {
     return delta_ <= other.delta_;
   }
-  bool operator>(TimeDelta other) const {
+  constexpr bool operator>(TimeDelta other) const {
     return delta_ > other.delta_;
   }
-  bool operator>=(TimeDelta other) const {
+  constexpr bool operator>=(TimeDelta other) const {
     return delta_ >= other.delta_;
   }
 
@@ -248,10 +248,14 @@ class BASE_EXPORT TimeDelta {
   // Constructs a delta given the duration in microseconds. This is private
   // to avoid confusion by callers with an integer constructor. Use
   // FromSeconds, FromMilliseconds, etc. instead.
-  explicit TimeDelta(int64_t delta_us) : delta_(delta_us) {}
+  constexpr explicit TimeDelta(int64_t delta_us) : delta_(delta_us) {}
 
   // Private method to build a delta from a double.
-  static TimeDelta FromDouble(double value);
+  static constexpr TimeDelta FromDouble(double value);
+
+  // Private method to build a delta from the product of a user-provided value
+  // and one of the kMicrosecondsPer constants.
+  static constexpr TimeDelta FromProduct(int64_t value, int microseconds_per);
 
   // Delta in microseconds.
   int64_t delta_;
@@ -578,60 +582,79 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
 // Inline the TimeDelta factory methods, for fast TimeDelta construction.
 
 // static
-inline TimeDelta TimeDelta::FromDays(int days) {
-  if (days == std::numeric_limits<int>::max())
-    return Max();
-  return TimeDelta(days * Time::kMicrosecondsPerDay);
+constexpr inline TimeDelta TimeDelta::FromDays(int days) {
+  return days == std::numeric_limits<int>::max()
+             ? Max()
+             : TimeDelta(days * Time::kMicrosecondsPerDay);
 }
 
 // static
-inline TimeDelta TimeDelta::FromHours(int hours) {
-  if (hours == std::numeric_limits<int>::max())
-    return Max();
-  return TimeDelta(hours * Time::kMicrosecondsPerHour);
+constexpr inline TimeDelta TimeDelta::FromHours(int hours) {
+  return hours == std::numeric_limits<int>::max()
+             ? Max()
+             : TimeDelta(hours * Time::kMicrosecondsPerHour);
 }
 
 // static
-inline TimeDelta TimeDelta::FromMinutes(int minutes) {
-  if (minutes == std::numeric_limits<int>::max())
-    return Max();
-  return TimeDelta(minutes * Time::kMicrosecondsPerMinute);
+constexpr inline TimeDelta TimeDelta::FromMinutes(int minutes) {
+  return minutes == std::numeric_limits<int>::max()
+             ? Max()
+             : TimeDelta(minutes * Time::kMicrosecondsPerMinute);
 }
 
 // static
-inline TimeDelta TimeDelta::FromSeconds(int64_t secs) {
-  return TimeDelta(secs) * Time::kMicrosecondsPerSecond;
+constexpr inline TimeDelta TimeDelta::FromSeconds(int64_t secs) {
+  return FromProduct(secs, Time::kMicrosecondsPerSecond);
 }
 
 // static
-inline TimeDelta TimeDelta::FromMilliseconds(int64_t ms) {
-  return TimeDelta(ms) * Time::kMicrosecondsPerMillisecond;
+constexpr inline TimeDelta TimeDelta::FromMilliseconds(int64_t ms) {
+  return FromProduct(ms, Time::kMicrosecondsPerMillisecond);
 }
 
 // static
-inline TimeDelta TimeDelta::FromSecondsD(double secs) {
+constexpr inline TimeDelta TimeDelta::FromSecondsD(double secs) {
   return FromDouble(secs * Time::kMicrosecondsPerSecond);
 }
 
 // static
-inline TimeDelta TimeDelta::FromMillisecondsD(double ms) {
+constexpr inline TimeDelta TimeDelta::FromMillisecondsD(double ms) {
   return FromDouble(ms * Time::kMicrosecondsPerMillisecond);
 }
 
 // static
-inline TimeDelta TimeDelta::FromMicroseconds(int64_t us) {
+constexpr inline TimeDelta TimeDelta::FromMicroseconds(int64_t us) {
+  // Note that TimeDelta::FromMicroseconds(numeric_limits<int64_t>::min()) is

[Jeffrey Yasskin, 2016/05/16 18:59:17]

The fact that this function is happy to produce values less than the minimum
might indicate that FromDouble() should just use saturated_cast and accept that
TimeDelta::FromMillisecondsD(-pow(2, 63)) < -TimeDelta::Max(). What do the
owners think?

[danakj, 2016/05/17 20:28:29]

Hm this is rather awkward. FromDouble appears to be the only use of -Max() in
the existing codebase outside of tests. I took a look for other callers using
Max() in weird ways and I don't see places that depend on FromDouble returning
-Max().

Which led me to wonder if it shouldn't return numeric_limits::min() instead of
-max? (Though, again we should probably change behaviour independently of making
things constexpr.)

[Jeffrey Yasskin, 2016/05/17 20:48:51]

The other use is in FromCheckedNumeric() but spells it
-std::numeric_limits<int64_t>::max(). I don't have an opinion on the right
behavior, but would like to change that in a dedicated CL so the constexpr stuff
doesn't get rolled back if it introduces a bug.

+  // less than the value returned when an operation underflows.
   return TimeDelta(us);
 }
 
 // static
-inline TimeDelta TimeDelta::FromDouble(double value) {
-  double max_magnitude = std::numeric_limits<int64_t>::max();
-  TimeDelta delta = TimeDelta(static_cast<int64_t>(value));
-  if (value > max_magnitude)
-    delta = Max();
-  else if (value < -max_magnitude)
-    delta = -Max();
-  return delta;
+constexpr inline TimeDelta TimeDelta::FromDouble(double value) {
+  // IsValueInRangeForNumericType is needed instead of a simple comparison
+  // against numeric_limits<int64_t>::max() because
+  // numeric_limits<int64_t>::max() is 2**63 - 1, which rounds to 2**63
+  // when comparing against a double value. (The largest int64_t that converts
+  // to a double value distinct from 2**63 is 2**63-1024.)
+  // static_cast<int64_t>(2**63) is undefined behavior per [conv.fpint] because
+  // the double is out of int64_t's range. However,
+  // IsValueInRangeForNumericType() isn't sufficient because it'll allow -2**63
+  // through, which is less than -Max().
+  return IsValueInRangeForNumericType<int64_t>(value) &&
+                 static_cast<int64_t>(value) >=
+                     -std::numeric_limits<int64_t>::max()
+             ? TimeDelta(static_cast<int64_t>(value))
+             : value > 0 ? Max() : -Max();
+}
+
+// static
+constexpr inline TimeDelta TimeDelta::FromProduct(int64_t value,
+                                                  int microseconds_per) {
+  return value > std::numeric_limits<int64_t>::max() / microseconds_per

[danakj, 2016/05/17 20:28:29]

Why do we want to make bad multiplications so well-behaved here? Is this related
to constexpr in some way I'm missing? If not I think we should do constexpr
without changing behaviour and if we want to fix overflow do that separately.

[Jeffrey Yasskin, 2016/05/17 20:48:51]

The old behavior of FromSeconds() and FromMilliseconds() used CheckedNumeric to
catch overflow and replace it with Max() or -Max():
https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.cc....
Since CheckedNumeric isn't constexpr, to maintain the behavior, I reproduced the
relevant part here.

I could split out the improvement to FromDouble if you like. That's not related
to preserving the old behavior.

[danakj, 2016/05/17 21:17:36]

Ah, operator* was doing that. I see, and the other times we were using a cast
from double to int and comparing, and things are bad.
I think it's a bit better as you say to change behaviour separately in case it
causes a problem, so just do the wrong thing with cast in FromDouble, and put a
TODO and make another CL to make things better.

The question is what's better. I don't like that we're doing this limit checking
in 2 different ways, in very different places. We could roll all our own math
here in one place, using Max() always instead of numeric_limits? But writing
this math is also not awesome.

Using saturated_cast consistently seems like the nicest of options, for all of
the cases, addition/subtraction/multiplication. We probably want to expose Min()
then for people to use if they want what -Max() would have done before. There's
a comment in time.cc about why we use max/-max instead of max/min, but I don't
really parse it to understand why it wouldn't be feasible.

So.. tl;dr I think my POV is:

1. Leave behaviour exactly the same even tho it's bad. FromProduct here is fine.
Put some TODOs to delete FromProduct, saturated math, and fix FromDouble.
2. Use saturated cast instead of doing this type of math ourselves. Delete the
checked math stuff and FromProduct.

+             ? Max()
+             : value < -std::numeric_limits<int64_t>::max() / microseconds_per
+                   ? -Max()
+                   : TimeDelta(value * microseconds_per);
 }
 
 // For logging use only.