Base: Make TimeDelta constructors deal with overflow.

Base: Make TimeDelta constructors deal with overflow.

BUG=466445
TBR=skuhne@chromium.org, enne@chromium.org, jochen@chromium.org, kbr@chromium.org

Committed: https://crrev.com/15462e858ebf34721e28a023b0b670c2011183ac
Cr-Commit-Position: refs/heads/master@{#339605}
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0a0885005e65d6ae51c6f0db51200a5de1dfde70
Cr-Commit-Position: refs/heads/master@{#340541}

[rvargas (doing something else), 2015-07-17 00:02:48]

rvargas@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-17 17:00:24]

lgtm if the reason why this works is because TimeDelta (or Time) has saturating
operator+, -, *.

https://codereview.chromium.org/1229853004/diff/1/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time.cc#newcode147
base/time/time.cc:147: return Time(kTimeTToMicrosecondsOffset) +
TimeDelta::FromSeconds(tt);
I like the changes in this file :-)

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode603
base/time/time.h:603: // Special-case max.
The previous comment answered "why?". From what I understand, it did so
incorrectly, but the intention was good.

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode606
base/time/time.h:606: return TimeDelta(days * Time::kMicrosecondsPerDay);
doesn't days * Time::kMicrosecondsPerDay overflow too if days is large but less
than max()?

…ah, kMicrosecondsPerDay is a int64, I see.

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode627
base/time/time.h:627: return TimeDelta(secs) * Time::kMicrosecondsPerSecond;
Does TimeDelta have a saturating operator*?

https://codereview.chromium.org/1229853004/diff/1/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time_unittest.cc#...
base/time/time_unittest.cc:878: EXPECT_TRUE(IsMin(t));
For giggles, can you add a

 
EXPECT_FALSE(IsMin(-TimeDelta::FromMicroseconds(std::numeric_limits<int64>::min())));

? (Admittedly only tangentially related to the CL at hand.)

[rvargas (doing something else), 2015-07-17 19:18:10]

Thanks

Let me know if you want another comment or if you want me to look at the
callers.

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode603
base/time/time.h:603: // Special-case max.
On 2015/07/17 17:00:23, Nico wrote:
> The previous comment answered "why?". From what I understand, it did so
> incorrectly, but the intention was good.

yeah... I actually don't like this. FromDays(max_int) returns Max, but
FromDays(max_int - 1) doesn't, and neither value is really overflowing anything.
However I'm afraid of code somewhere relying on that behavior :(

So the real reason of "why" today is "backwards compatibility" (or actually,
fear of breaking things), even though that was never part of the contract.

I could go through the 746 call sites that I see. (+388 FromHours + 464
FromMinutes)

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode627
base/time/time.h:627: return TimeDelta(secs) * Time::kMicrosecondsPerSecond;
On 2015/07/17 17:00:23, Nico wrote:
> Does TimeDelta have a saturating operator*?

Yes, all TimeDelta operators should do the right thing on overflow.

https://codereview.chromium.org/1229853004/diff/1/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time_unittest.cc#...
base/time/time_unittest.cc:878: EXPECT_TRUE(IsMin(t));
On 2015/07/17 17:00:24, Nico wrote:
> For giggles, can you add a
> 
>  
>
EXPECT_FALSE(IsMin(-TimeDelta::FromMicroseconds(std::numeric_limits<int64>::min())));
> 
> ? (Admittedly only tangentially related to the CL at hand.)

Will do.

[Nico, 2015-07-17 20:21:20]

(ok to land)

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1229853004/diff/1/base/time/time.h#newcode603
base/time/time.h:603: // Special-case max.
On 2015/07/17 19:18:10, rvargas wrote:
> On 2015/07/17 17:00:23, Nico wrote:
> > The previous comment answered "why?". From what I understand, it did so
> > incorrectly, but the intention was good.
> 
> yeah... I actually don't like this. FromDays(max_int) returns Max, but
> FromDays(max_int - 1) doesn't, and neither value is really overflowing
anything.
> However I'm afraid of code somewhere relying on that behavior :(
> 
> So the real reason of "why" today is "backwards compatibility" (or actually,
> fear of breaking things), even though that was never part of the contract.
> 
> I could go through the 746 call sites that I see. (+388 FromHours + 464
> FromMinutes)

Hm, maybe just remove the comment then I guess. As is, it just repeats the line
below.

[rvargas (doing something else), 2015-07-21 00:32:16]

The CQ bit was checked by rvargas@chromium.org

[rvargas (doing something else), 2015-07-21 00:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1229853004/#ps20001
(title: "Add test and remove some comments.")

[commit-bot: I haz the power, 2015-07-21 00:33:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229853004/20001

[commit-bot: I haz the power, 2015-07-21 03:06:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-21 03:07:37]

Patchset 2 (id:??) landed as
https://crrev.com/15462e858ebf34721e28a023b0b670c2011183ac
Cr-Commit-Position: refs/heads/master@{#339605}

[Denis Kuznetsov (DE-MUC), 2015-07-21 12:31:48]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1250453004/ by antrim@chromium.org.

The reason for reverting is: Reverting CL: it seems to cause regression in
number of static initializers.
For details see https://code.google.com/p/chromium/issues/detail?id=512348
.

[Denis Kuznetsov (DE-MUC), 2015-07-21 14:18:32]

Confirm that reverting this CL removes regression.
It seems that there are some TimeDelta static constants in code, and FromDouble
is too complex for compiler, so we get actual static initializer as result.

[rvargas (doing something else), 2015-07-22 02:39:40]

What do you think about patch set 3?

[rvargas (doing something else), 2015-07-24 21:57:44]

On 2015/07/22 02:39:40, rvargas wrote:
> What do you think about patch set 3?

Nico: gentle ping.

[Nico, 2015-07-24 22:07:04]

lgtm

[rvargas (doing something else), 2015-07-24 23:09:40]

rvargas@chromium.org changed reviewers:
+ enne@chromium.org, jochen@chromium.org, kbr@chromium.org, skuhne@chromium.org

[rvargas (doing something else), 2015-07-24 23:10:31]

Thanks.

+TBR owners outside of base.

[Ken Russell (switch to Gerrit), 2015-07-25 00:21:53]

ui/gl/gl_surface_glx.cc lgtm

[rvargas (doing something else), 2015-07-27 18:26:23]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-07-27 18:27:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229853004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1229853004/40001

[commit-bot: I haz the power, 2015-07-27 20:09:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-27 20:10:14]

Patchset 3 (id:??) landed as
https://crrev.com/0a0885005e65d6ae51c6f0db51200a5de1dfde70
Cr-Commit-Position: refs/heads/master@{#340541}