Use NaN/Inf to represent unresolved/indefinite SMILTimes

Source/core/svg/animation/SMILTime.h

Index: Source/core/svg/animation/SMILTime.h
diff --git a/Source/core/svg/animation/SMILTime.h b/Source/core/svg/animation/SMILTime.h
index cab9fe9d6123e19c3893b5da31c136c6a750c8f2..5cb8b5ed6ad88a137e6a6dac2064d1379e2f1d56 100644
--- a/Source/core/svg/animation/SMILTime.h
+++ b/Source/core/svg/animation/SMILTime.h
@@ -28,28 +28,38 @@
 
 #include "wtf/Assertions.h"
 #include "wtf/MathExtras.h"
+#include "wtf/StdLibExtras.h"
 
 namespace blink {
 
 class SMILTime {
 public:
     SMILTime() : m_time(0) { }
-    SMILTime(double time) : m_time(time) { ASSERT(!std::isnan(time)); }
+    SMILTime(double time) : m_time(time) { }
     SMILTime(const SMILTime& o) : m_time(o.m_time) { }

[fs, 2014/08/19 21:49:43]

While you're here... I believe this could be dropped (it's the same as the
default).

[reni, 2014/08/21 14:20:06]

Done.

 
-    static SMILTime unresolved() { return unresolvedValue; }
-    static SMILTime indefinite() { return indefiniteValue; }
+    static SMILTime unresolved() { return unresolvedValue(); }
+    static SMILTime indefinite() { return indefiniteValue(); }
 
     SMILTime& operator=(const SMILTime& o) { m_time = o.m_time; return *this; }

[fs, 2014/08/19 21:49:43]

This should be possible to drop as well.

[reni, 2014/08/21 14:20:06]

Done.

     double value() const { return m_time; }
 
-    bool isFinite() const { return m_time < indefiniteValue; }
-    bool isIndefinite() const { return m_time == indefiniteValue; }
-    bool isUnresolved() const { return m_time == unresolvedValue; }
+    bool isFinite() const { return m_time < indefiniteValue(); }

[fs, 2014/08/19 21:49:43]

Use std::isfinite here (hopefully that also dropping some of the other changes,
because as is this will return 'false' for the unresolved value (NaN)).

[reni, 2014/08/21 14:20:06]

Done.

+    bool isIndefinite() const { return std::isinf(m_time); }
+    bool isUnresolved() const { return std::isnan(m_time); }
 
 private:
-    static const double unresolvedValue;
-    static const double indefiniteValue;
+    static const double& unresolvedValue()

[fs, 2014/08/19 21:49:43]

I'd suggest instead just have unresolved()/indefinite() (above) return the
std::numeric_limits<...>::foo() directly. I don't think there's a need for these
- I'd suspect the std-methods just return the appropriate bit-pattern (should
maybe confirm that though...).

[reni, 2014/08/21 14:20:06]

Done.

+    {
+        DEFINE_STATIC_LOCAL(double, unresolvedValue, (std::numeric_limits<double>::quiet_NaN()));
+        return unresolvedValue;
+    }
+
+    static const double& indefiniteValue()
+    {
+        DEFINE_STATIC_LOCAL(double, indefiniteValue, (std::numeric_limits<double>::infinity()));
+        return indefiniteValue;
+    }
 
     double m_time;
 };
@@ -88,14 +98,75 @@ struct SMILInterval {
     SMILTime end;
 };
 
-inline bool operator==(const SMILTime& a, const SMILTime& b) { return a.isFinite() && a.value() == b.value(); }
-inline bool operator!(const SMILTime& a) { return !a.isFinite() || !a.value(); }
-inline bool operator!=(const SMILTime& a, const SMILTime& b) { return !operator==(a, b); }
-inline bool operator>(const SMILTime& a, const SMILTime& b) { return a.value() > b.value(); }
-inline bool operator<(const SMILTime& a, const SMILTime& b) { return a.value() < b.value(); }
-inline bool operator>=(const SMILTime& a, const SMILTime& b) { return a.value() > b.value() || operator==(a, b); }
-inline bool operator<=(const SMILTime& a, const SMILTime& b) { return a.value() < b.value() || operator==(a, b); }
-inline bool operator<(const SMILTimeWithOrigin& a, const SMILTimeWithOrigin& b) { return a.time() < b.time(); }
+inline bool operator==(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return true;
+    return a.isFinite() && a.value() == b.value();

[fs, 2014/08/19 21:49:43]

I think the following would work equal well:

a.isUnresolved() == b.isUnresolved() || a.value() == b.value()

even though it's not equivalent, since 'indefinite' is special-cased (but only
in one of the operands...).

So, please try the above and see if there's tests complaining...

[fs, 2014/08/19 22:27:14]

s/==/&&/

[reni, 2014/08/21 14:20:06]

Done.

+}
+
+inline bool operator!(const SMILTime& a)
+{
+    return !a.isFinite() || !a.value();

[fs, 2014/08/19 21:49:43]

It's _probably_ possible to simplify this (to just the right-hand side), but no
rush I suppose.

+}
+
+inline bool operator!=(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return false;
+    if (a.isUnresolved() || b.isUnresolved())
+        return true;
+    return !operator==(a, b);

[fs, 2014/08/19 21:49:43]

This really ought to be possible to have as it previously was.

[reni, 2014/08/21 14:20:06]

Done.

+}
+
+inline bool operator>(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return false;
+    if (a.isUnresolved())
+        return true;
+    if (b.isUnresolved())
+        return false;
+    return a.value() > b.value();

[fs, 2014/08/19 21:49:43]

Make this:

a.isUnresolved() || a.value() > b.value()

(I'm not sure why we need the left-hand side of that though, but we can leave
that for later...)

[reni, 2014/08/21 14:20:06]

Done.

[fs, 2014/08/21 15:30:39]

It dawned on me why we want the LHS here. In general we want to get an ordering
so that:

finite < indefinite < unresolved

and hence:

isfinite(v) < Inf < NaN

(compare to previous representation: v < FLT_MAX < DBL_MAX)

and the first part is now handled by IEEE754 (i.e. operator< for double), but we
need the LHS to guarantee that NaN (unresolved) orders correctly. Maybe that's
worth noting in a comment somewhere around here...

[reni, 2014/08/21 20:52:31]

Done.

+}
+
+inline bool operator<(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return false;
+    if (a.isUnresolved())
+        return false;
+    if (b.isUnresolved())
+        return true;
+    return a.value() < b.value();

[fs, 2014/08/19 21:49:43]

Could either use the operator> above (with operands reversed), or be modified in
the same way as it.

[reni, 2014/08/21 14:20:06]

Done.

+}
+
+inline bool operator>=(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return true;
+    if (a.isUnresolved())
+        return true;
+    if (b.isUnresolved())
+        return false;
+    return a.value() > b.value() || operator==(a, b);

[fs, 2014/08/19 21:49:43]

This (and <=) could just use the operator> (operator<) from above and drop the
initial unresolved-checks.

[reni, 2014/08/21 14:20:06]

Done.

+}
+
+inline bool operator<=(const SMILTime& a, const SMILTime& b)
+{
+    if (a.isUnresolved() && b.isUnresolved())
+        return true;
+    if (a.isUnresolved())
+        return false;
+    if (b.isUnresolved())
+        return true;
+    return a.value() < b.value() || operator==(a, b);
+}
+
+inline bool operator<(const SMILTimeWithOrigin& a, const SMILTimeWithOrigin& b)
+{
+    return a.time() < b.time();
+}
 
 SMILTime operator+(const SMILTime&, const SMILTime&);
 SMILTime operator-(const SMILTime&, const SMILTime&);