Fixing Time::Max()'s behavior with Time::ToTimeT() and friends.

Fixing Time::Max()'s behavior with Time::ToTimeT() and friends.

It looks like the math that's done to convert epoch times between platforms is
giving incorrect results when confronted with a "max" time. This CL adjusts the
various conversion methods to persist maxness in the same way they currently
persist zeroness.

BUG=146328
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155591

[Mike West, 2012-09-04 11:36:10]

Hi Mark! As followup to the earlier addition of base::Time::Max(), would you
mind taking a look at this CL's adjustment to ToTimeT() and related functions?

It also adds Time::is_max() as a counterpart to Time::is_null() to enable
comparisons without including <limits> everywhere.

Thanks!

[Mark Mentovai, 2012-09-04 13:23:14]

https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h
File base/time.h (right):

https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h#newcode277
base/time.h:277: static Time FromTimeT(time_t tt);
Max preservation for time_t, WebKit double, and JS double, but…

https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h#newcode295
base/time.h:295: static Time FromTimeVal(struct timeval t);
no max preservation for this, or CFAbsoluteTime, or FILETIME?

[Mike West, 2012-09-04 13:46:21]

Ack. I completely missed the platform-specific bits.

It doesn't look like ToTimeVal or FILETIME actually preserves null at the
moment. Is there a reason for that, or is it just oversight? I'm happy to add it
in if it's accidental.

On 2012/09/04 13:23:14, Mark Mentovai wrote:
> https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h
> File base/time.h (right):
> 
> https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h#newcode277
> base/time.h:277: static Time FromTimeT(time_t tt);
> Max preservation for time_t, WebKit double, and JS double, but…
> 
> https://chromiumcodereview.appspot.com/10916089/diff/1/base/time.h#newcode295
> base/time.h:295: static Time FromTimeVal(struct timeval t);
> no max preservation for this, or CFAbsoluteTime, or FILETIME?

[Mark Mentovai, 2012-09-04 13:49:42]

I’m not sure. I think it’s just a matter of some platform-specific conversions
following the established pattern and others not. It’s probably best for
everyone to agree on the same rules.

[Mike West, 2012-09-04 13:54:15]

On 2012/09/04 13:49:42, Mark Mentovai wrote:
> I’m not sure. I think it’s just a matter of some platform-specific conversions
> following the established pattern and others not. It’s probably best for
> everyone to agree on the same rules.

Alright. Then I'll dive into some scary-looking bit_casting and throw you a
patch in a bit. Thanks! :)

[Mike West, 2012-09-05 05:51:00]

On 2012/09/04 13:54:15, Mike West (chromium) wrote:
> On 2012/09/04 13:49:42, Mark Mentovai wrote:
> > I’m not sure. I think it’s just a matter of some platform-specific
conversions
> > following the established pattern and others not. It’s probably best for
> > everyone to agree on the same rules.
> 
> Alright. Then I'll dive into some scary-looking bit_casting and throw you a
> patch in a bit. Thanks! :)

I think this patch is correct, but I'm not sure where to put tests for the
platform-specific time conversion code. None of it seems to be tested at the
moment... :(

WDYT?

[Mark Mentovai, 2012-09-05 15:27:53]

It’d be best if the platform-specific parts were tested too.

http://codereview.chromium.org/10916089/diff/12001/base/time.cc
File base/time.cc (right):

http://codereview.chromium.org/10916089/diff/12001/base/time.cc#newcode78
base/time.cc:78: return Max(); // Max is max.
Formatting: you have all of these new “max is max” comments, which I guess are
fine, but you’re supposed to put at least two spaces between the last
non-whitespace non-comment thing on each line (the ;) and the //. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
.

http://codereview.chromium.org/10916089/diff/12001/base/time.cc#newcode122
base/time.cc:122: return 0; // Preserve 0 so the invalid result doesn't depend
on a platform.
Dunno why you’ve changed “the” to “a.” Seems kinda awkward now.

http://codereview.chromium.org/10916089/diff/12001/base/time_mac.cc
File base/time_mac.cc (right):

http://codereview.chromium.org/10916089/diff/12001/base/time_mac.cc#newcode57
base/time_mac.cc:57: if (static_cast<int64>(t) ==
std::numeric_limits<int64>::max())
This is very weird. Did std::numeric_limits<CFAbsoluteTime>::max() not work for
some reason?

CFAbsoluteTime is a double under the covers.

http://codereview.chromium.org/10916089/diff/12001/base/time_mac.cc#newcode68
base/time_mac.cc:68: return
static_cast<CFAbsoluteTime>(std::numeric_limits<int64>::max());
Same.

http://codereview.chromium.org/10916089/diff/12001/base/time_posix.cc
File base/time_posix.cc (right):

http://codereview.chromium.org/10916089/diff/12001/base/time_posix.cc#newcode271
base/time_posix.cc:271: if (t.tv_usec ==
static_cast<int>(Time::kMicrosecondsPerSecond) - 1 &&
For correctness, you should use the type of the tv_usec field here, suseconds_t,
instead of int.

http://codereview.chromium.org/10916089/diff/12001/base/time_posix.cc#newcode272
base/time_posix.cc:272: t.tv_sec == std::numeric_limits<int64>::max())
And the type of the tv_sec field, time_t, here.

http://codereview.chromium.org/10916089/diff/12001/base/time_posix.cc#newcode288
base/time_posix.cc:288: result.tv_sec = std::numeric_limits<int64>::max();
Again on these two lines.

http://codereview.chromium.org/10916089/diff/12001/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/10916089/diff/12001/base/time_unittest.cc#newc...
base/time_unittest.cc:114: #if defined(OS_POSIX)
Here’s an example of a platform-specific test.

[Mike West, 2012-09-05 20:25:14]

As is likely obvious, I don't do much platform-specific work. Thanks for working
through the review with me. :)

WDYT of the current iteration?

http://codereview.chromium.org/10916089/diff/12001/base/time.cc
File base/time.cc (right):

http://codereview.chromium.org/10916089/diff/12001/base/time.cc#newcode122
base/time.cc:122: return 0; // Preserve 0 so the invalid result doesn't depend
on a platform.
On 2012/09/05 15:27:53, Mark Mentovai wrote:
> Dunno why you’ve changed “the” to “a.” Seems kinda awkward now.

80 columns. :) I'll wrap it in {} instead.

[msarda, 2012-09-06 15:04:34]

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time.cc
File base/time.cc (right):

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time.cc#newcode89
base/time.cc:89: return (us_ - kTimeTToMicrosecondsOffset) /
kMicrosecondsPerSecond;
There is still a risk of overflow here: us_ might still be big enough without
actually being equal to std::numeric_limits<int64>::max().
I would add a DLOG(WARNING) in case there is an overflow and return
std::numeric_limits<time_t>::max().

[Mark Mentovai, 2012-09-06 16:10:49]

msarda also raises an interesting point.

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest.cc
File base/time_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest....
base/time_unittest.cc:512: EXPECT_EQ(std::numeric_limits<time_t>::max(),
t.ToTimeVal().tv_sec);
Don’t call ToTimeVal twice.

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest....
base/time_unittest.cc:529: EXPECT_EQ(std::numeric_limits<int>::max(),
t.ToFileTime().dwHighDateTime);
And don’t call ToFileTime twice.

[Mike West, 2012-09-06 19:51:16]

Thanks to you both.

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time.cc
File base/time.cc (right):

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time.cc#newcode89
base/time.cc:89: return (us_ - kTimeTToMicrosecondsOffset) /
kMicrosecondsPerSecond;
On 2012/09/06 15:04:34, msarda wrote:
> There is still a risk of overflow here: us_ might still be big enough without
> actually being equal to std::numeric_limits<int64>::max().
> I would add a DLOG(WARNING) in case there is an overflow and return
> std::numeric_limits<time_t>::max().

Good point. I've added the log. This one looks like the only one that can
overflow based on the offset. The rest of the conversions are doubles, or on
platforms where the offset is 0.

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest.cc
File base/time_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest....
base/time_unittest.cc:512: EXPECT_EQ(std::numeric_limits<time_t>::max(),
t.ToTimeVal().tv_sec);
On 2012/09/06 16:10:49, Mark Mentovai wrote:
> Don’t call ToTimeVal twice.

Done.

https://chromiumcodereview.appspot.com/10916089/diff/4007/base/time_unittest....
base/time_unittest.cc:529: EXPECT_EQ(std::numeric_limits<int>::max(),
t.ToFileTime().dwHighDateTime);
On 2012/09/06 16:10:49, Mark Mentovai wrote:
> And don’t call ToFileTime twice.

Done.

[msarda, 2012-09-07 07:58:20]

LGTM.

[Mark Mentovai, 2012-09-07 12:23:39]

LGTM

[commit-bot: I haz the power, 2012-09-08 07:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10916089/14010

[commit-bot: I haz the power, 2012-09-08 12:49:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10916089/14010

[commit-bot: I haz the power, 2012-09-08 17:50:09]

Change committed as 155591