Alexey Serbin has posted comments on this change.

Change subject: [util/monotime] added handy operators for MonoTime
......................................................................


Patch Set 1:

(12 comments)

Will post a new version shortly.

http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime-test.cc
File src/kudu/util/monotime-test.cc:

Line 211:     MonoTime m_end = start.AddDelta(MonoDelta(-delta.nano_delta_));
> Use ToNanoseconds() to avoid test friendship?
OK, if you think it's a good idea, I'll do.  My original thought was that it's 
internal implementation detail, and as I understand there is no guarantee that 
nanoseconds is the actual granularity of MonoDelta.  Using the private 
constructor that works with internals looked more compelling to me.


http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.cc
File src/kudu/util/monotime.cc:

Line 209:   // A sanity check: nanos_ is unsigned
> I don't understand the second check. If 'delta' is negative, why must its p
The second check is to ensure that abs(delta.nano_delta) <= nanos_ if 
delta.nano_delta is negative to prevent things like uint64_t x = 100UL - 200UL. 
 If it makes things clearer, I can replace (-delta.nano_delta <= nanos_) with 
(delta.nano_delta < 0 || abs(delta.nano_delta) <= nanos)


Line 210:   DCHECK(delta.nano_delta_ >= 0 || -delta.nano_delta_ <= nanos_);
> Can't you call delta.ToNanoseconds() to extract the nanos, instead of using
Yep, will do -- thanks.  I explained my motivation for not using it in one of 
the earlier comments.


Line 235:   this->AddDelta(MonoDelta(-1 * delta.nano_delta_));
> Can use delta.ToNanoseconds() here.
Done


Line 244:   return !(*this == other);
> How about !this->Equals(other)? Seems easier to compare with operator==.
Yep, I think it's subjective -- to me it looked otherwise :) Will replaceq.


Line 271:   DCHECK(ts.tv_nsec >= 0 || -ts.tv_nsec <= nanos_);
> Again, I don't understand the second half of this check.
I'll copy-paste the explanation here:

The second check is to ensure that abs(delta.nano_delta) <= nanos_ if 
delta.nano_delta is negative to prevent things like uint64_t x = 100UL - 200UL. 
 If it makes things clearer, I can replace (-delta.nano_delta <= nanos_) with 
(delta.nano_delta < 0 || abs(delta.nano_delta) <= nanos)


Line 299:   return MonoTime(t).AddDelta(MonoDelta(-delta.nano_delta_));
> Can use ToNanoseconds() here.
Done


http://gerrit.cloudera.org:8080/#/c/3999/1/src/kudu/util/monotime.h
File src/kudu/util/monotime.h:

Line 201:   MonoTime& AddDelta(const MonoDelta &delta);
> Hmm, is this ABI compatible with the old AddDelta()? It may not matter; the
Good catch!  I'll remove this modification since it could break 
backward-compatibility.

As for the motivation, look at the MonoTime operator +(const MonoTime& t, const 
MonoDelta& delta;

Also, I wanted to write one-line code like
MonoTime& MonoTime::operator +=(const MonoDelta& delta) {
  return this->AddDelta(delta);
}

Another point was before I added those operators: it came from the code which 
looked like the following:

const MonoDelta delta(MonoDelta::FromMilliseconds(MAX_WAIT_MS));
const MonoTime tmp(MonoTime::Now());

const MonoTime deadline_0(tmp.AddDelta(delta));
const MonoTime deadline_1(tmp.AddDelta(delta));


Line 232:   MonoTime& operator +=(const MonoDelta& delta);
> Can you use "operator<type>" instead of "operator <type>" (note the lack of
Done


Line 251:   /// Check if the object represents a different point in time.
> Nit: "...than the given one."
Done


Line 321: MonoDelta KUDU_EXPORT operator -(const MonoTime& t0, const MonoTime& 
t1);
> The Google style guide frowns on operator overloading when it's incomplete 
The motivation for this was the code like this:

MonoDelta t_diff = t_end - t_begin;
EXPECT_GT(wait_timeout_ms / 10, t_diff.ToMilliseconds());

This is because I was confused by MonoTime::GetDeltaSince(): should I write 
t_end.GetDeltaSince(t_begin) or t_begin.GetDeltaSince(t_end) to get the desired 
result?  Using the operator clears those questions once and for all.


Line 323: /// Add the specified interval to the given point in time.
> Nit: "time interval".
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3999
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8a336bc79461adebfb2d5c136d71f90efcd36db
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to