Create TimeBase for time related classes.

Create TimeBase for time related classes.

Currently we have Time and TimeTicks sharing some methods. This patch
creates TimeBase, and makes Time and TimeTicks inherits from it, so that time
related classes won't have to implement common methods and it's easier to
introduce new time related classes.

BUG=v8:4990
LOG=n

Committed: https://crrev.com/5cbe0f5d25592525b1f8244a81c75a9df29969a8
Cr-Commit-Position: refs/heads/master@{#36088}

[lpy, 2016-05-04 22:51:43]

lpy@chromium.org changed reviewers:
+ bmeurer@chromium.org, fmeawad@chromium.org, jochen@chromium.org

[lpy, 2016-05-04 22:51:43]

PTAL.

[fmeawad, 2016-05-04 23:34:22]

It is worth mentioning that this work is triggered by the need to add
ThreadTicks type to maintain the CPU time. Instead of creating a third Time
class, we will group the common functionalities in TimeBase instead. (Same as
chromium)

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

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.cc#n...
src/base/platform/time.cc:33: int64_t SaturatedAdd(TimeDelta delta, int64_t
value) {
The Saturated methods does change the functionality, it is OK to include in this
CL as long as it is part of the purpose of the CL.

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

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:211: bool IsMax() const { return us_ ==
std::numeric_limits<int64_t>::max(); }
This method only exists in Time and not TimeTicks, not sure if it makes sense in
the base class.

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:286: class Time : public time_internal::TimeBase<Time>
{
Should this still be final?

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:348: class TimeTicks : public
time_internal::TimeBase<TimeTicks> {
ditto

[lpy, 2016-05-05 00:00:05]

PTAL.

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

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:211: bool IsMax() const { return us_ ==
std::numeric_limits<int64_t>::max(); }
On 2016/05/04 23:34:22, fmeawad wrote:
> This method only exists in Time and not TimeTicks, not sure if it makes sense
in
> the base class.

Acknowledged.

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:286: class Time : public time_internal::TimeBase<Time>
{
On 2016/05/04 23:34:22, fmeawad wrote:
> Should this still be final?

Done.

https://codereview.chromium.org/1952843002/diff/1/src/base/platform/time.h#ne...
src/base/platform/time.h:348: class TimeTicks : public
time_internal::TimeBase<TimeTicks> {
On 2016/05/04 23:34:21, fmeawad wrote:
> ditto

Done.

[fmeawad, 2016-05-05 00:01:40]

lgtm, but I want to hear from the V8 team concerning the Saturated functions.

[Benedikt Meurer, 2016-05-06 04:15:06]

https://codereview.chromium.org/1952843002/diff/20001/src/base/platform/time.h
File src/base/platform/time.h (right):

https://codereview.chromium.org/1952843002/diff/20001/src/base/platform/time....
src/base/platform/time.h:34: int64_t SaturatedAdd(TimeDelta delta, int64_t
value);
Can you move these to base/bits.{h,cc} instead as

int64_t SignedSaturatedAdd64(int64_t, int64_t)
int64_t SignedSaturatedSub64(int64_t, int64_t)

and use that for the time classes instead?

[lpy, 2016-05-06 05:09:08]

I updated the CL.

PTAL.

https://codereview.chromium.org/1952843002/diff/20001/src/base/platform/time.h
File src/base/platform/time.h (right):

https://codereview.chromium.org/1952843002/diff/20001/src/base/platform/time....
src/base/platform/time.h:34: int64_t SaturatedAdd(TimeDelta delta, int64_t
value);
On 2016/05/06 04:15:06, Benedikt Meurer wrote:
> Can you move these to base/bits.{h,cc} instead as
> 
> int64_t SignedSaturatedAdd64(int64_t, int64_t)
> int64_t SignedSaturatedSub64(int64_t, int64_t)
> 
> and use that for the time classes instead?

Done.

[Benedikt Meurer, 2016-05-06 05:12:58]

Thanks. LGTM once comments are addressed.

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h#newcode308
src/base/bits.h:308: CheckedNumeric<int64_t> rv(lhs);
Please move the implementation and therefore the use of CheckedNumeric into the
bits.cc file.

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h#newcode317
src/base/bits.h:317: CheckedNumeric<int64_t> rv(lhs);
See comment above.

[lpy, 2016-05-06 05:28:29]

Thanks. Comments were addressed.
jochen@, PTAL.

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h#newcode308
src/base/bits.h:308: CheckedNumeric<int64_t> rv(lhs);
On 2016/05/06 05:12:58, Benedikt Meurer wrote:
> Please move the implementation and therefore the use of CheckedNumeric into
the
> bits.cc file.

Done.

https://codereview.chromium.org/1952843002/diff/40001/src/base/bits.h#newcode317
src/base/bits.h:317: CheckedNumeric<int64_t> rv(lhs);
On 2016/05/06 05:12:58, Benedikt Meurer wrote:
> See comment above.

Done.

[jochen (gone - plz use gerrit), 2016-05-06 09:43:02]

https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h#newcode20
src/base/bits.h:20: namespace internal {
safe_math.h has using internal::CheckedNumeric in namespace base, so why is this
needed here?

[lpy, 2016-05-06 17:23:23]

https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h
File src/base/bits.h (right):

https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h#newcode20
src/base/bits.h:20: namespace internal {
On 2016/05/06 09:43:02, jochen wrote:
> safe_math.h has using internal::CheckedNumeric in namespace base, so why is
this
> needed here?

It's used in the declaration of FromCheckedNumeric.

[jochen (gone - plz use gerrit), 2016-05-06 18:20:32]

On 2016/05/06 at 17:23:23, lpy wrote:
> https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h
> File src/base/bits.h (right):
> 
>
https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h#newcode20
> src/base/bits.h:20: namespace internal {
> On 2016/05/06 09:43:02, jochen wrote:
> > safe_math.h has using internal::CheckedNumeric in namespace base, so why is
this
> > needed here?
> 
> It's used in the declaration of FromCheckedNumeric.

Doesn't it work if you use just CheckedNuneric?

[lpy, 2016-05-06 18:26:07]

On 2016/05/06 18:20:32, jochen wrote:
> On 2016/05/06 at 17:23:23, lpy wrote:
> > https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h
> > File src/base/bits.h (right):
> > 
> >
>
https://codereview.chromium.org/1952843002/diff/80001/src/base/bits.h#newcode20
> > src/base/bits.h:20: namespace internal {
> > On 2016/05/06 09:43:02, jochen wrote:
> > > safe_math.h has using internal::CheckedNumeric in namespace base, so why
is
> this
> > > needed here?
> > 
> > It's used in the declaration of FromCheckedNumeric.
> 
> Doesn't it work if you use just CheckedNuneric?

No, it's a compilation error.

[jochen (gone - plz use gerrit), 2016-05-06 18:29:10]

lgtm

[lpy, 2016-05-06 18:34:07]

On 2016/05/06 18:29:10, jochen wrote:
> lgtm

thanks.

[lpy, 2016-05-06 18:34:11]

The CQ bit was checked by lpy@chromium.org

[lpy, 2016-05-06 18:34:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmeawad@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1952843002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-05-06 18:34:16]

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

[commit-bot: I haz the power, 2016-05-06 18:35:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-06 18:36:50]

Description was changed from

==========
Create TimeBase for time related classes.

Currently we have Time and TimeTicks sharing some methods. This patch
creates TimeBase, and makes Time and TimeTicks inherits from it, so that time
related classes won't have to implement common methods and it's easier to
introduce new time related classes.

BUG=v8:4990
LOG=n
==========

to

==========
Create TimeBase for time related classes.

Currently we have Time and TimeTicks sharing some methods. This patch
creates TimeBase, and makes Time and TimeTicks inherits from it, so that time
related classes won't have to implement common methods and it's easier to
introduce new time related classes.

BUG=v8:4990
LOG=n

Committed: https://crrev.com/5cbe0f5d25592525b1f8244a81c75a9df29969a8
Cr-Commit-Position: refs/heads/master@{#36088}
==========

[commit-bot: I haz the power, 2016-05-06 18:36:51]

Patchset 5 (id:??) landed as
https://crrev.com/5cbe0f5d25592525b1f8244a81c75a9df29969a8
Cr-Commit-Position: refs/heads/master@{#36088}