DateTime class relies on private members of other objects

[srawlins]

DateTime has a few methods that rely on a private member of objects:

bool isAfter(DateTime other) {
  return _value > other._value;
}

other is not guaranteed to be of a class declared in dart:core, so it's _value may be inaccessible.

In practice, this means that DateTime is not implementable, a problem for the timezone package:

new DateTime.now().isAfter(new TZDateTime.now(detroit))

throws trying to call _value on the TZDateTime object. The docs for _value read:

The value of this DateTime.

The content of this field is implementation dependent. On JavaScript it is equal to [millisecondsSinceEpoch]. On the VM it is equal to [microsecondsSinceEpoch].

I think the best solution is to make a public getter, value for _value. Especially when TZDateTime is used to wrap a native DateTime, and should set its own _value to the native DateTime's _value, so that it doesn't have to use a hack like if (identical(1, 1.0) to make up a value.

[floitschG]

The publicly accessible members for _value are millisecondsSinceEpoch and microSecondsSinceEpoch. I vote against making value publicly accessible.

However, isAfter (et al) could use those getters instead of accessing _value directly.

[srawlins]

I see. So all of the methods that access the _value of an other object now should become external in lib/core/date_time.dart, and their implementations will be moved to the VM/dart2js patches which will know to use either millisecondsSinceEpoch or microSecondsSinceEpoch. This amounts to:

• operator ==
• isBefore
• isAfter
• isAtSameMomentAs
• compareTo

Any code that implements DateTime can use if (identical(1, 1.0) to implement their own _value, for example for the purposes of hashCode, toUtc, etc. (Perhaps an implementation doesn't need to use the same _value for these methods, and so doesn't need to use if (identical(1, 1.0), but the trick is available if they do.)

[lrhn]

No need to go external, just change isAfter to be => microsecondsSinceEpoch > other.microsecondsSinceEpoch, and similarly for the rest. That would be correct and will work when combined with other implementations of the interface that do implement microseconds, even if the platform DateTime implementation uses only microsecond granularity.

Specifying hashCode is always problematic. If you want an interface to be implementable, you need to say how the hashCode is computed, but if you do so, you might pick a variant that is expensive or bad-at-hashing for some implementation (say, using microsecondsSinceEpoch & 0x3FFFFFFF might not be great if it only has millisecond precision, then the three lower bits will always be zero).
We haven't specified the hashCode yet, so that's still in the air.

[srawlins]

Ah, good idea, @lrhn. Two questions before I send a patch:

• microsecondsSinceEpoch is not slower for being a getter?

• microsecondsSinceEpoch doc notes:

  Note that this value does not fit into 53 bits (the size of a IEEE double). A JavaScript number is not able to hold this value.

  Does this not get in the way?

[lrhn]

It probably does get in the way, but if the compare must be ready to support non-platform DateTime methods, it needs to account for microseconds some way. Maybe it's better to do:

bool isAfter(DateTime other) { 
   var ms = this.millisecondsSinceEpoch;
   var oms = other.millisecondsSinceEpoch;
   return ms > oms || (ms == oms && this.microseconds > other.microseconds);
}

but the implementation must check the microseconds of the other.

And sure, if we can be more efficient by doing it differently on different platforms, we can go the external way and have independent implementations, but doubling the code to maintain should only be done when we can see that there is an actual need for the performance.

[lrhn]

DateTime no longer depends on _value of other objects.

[srawlins]

Thanks much!