Adar Dembo has posted comments on this change.

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


Patch Set 1:

(12 comments)

Is the use case for these guys very compelling? Although the Google style guide 
permits operator overloading (subject to some conditions), we're generally 
pretty judicious with our use of it. I'm not opposed to this patch, but I think 
we should have a pretty compelling set of use cases to merit merging it.

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?


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 
positive version be less than or equal to 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 
friendship?


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


Line 244:   return !(*this == other);
How about !this->Equals(other)? Seems easier to compare with operator==.


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


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


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 
client API never uses MonoTimes, just MonoDeltas. But I'm still curious.

What's the motivation here, anyway? Support fluent-style AddDelta() chaining? 
Is that really something we need?


Line 232:   MonoTime& operator +=(const MonoDelta& delta);
Can you use "operator<type>" instead of "operator <type>" (note the lack of 
space"? I think that'd be more consistent with overloaded operators elsewhere 
in the codebase.


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


Line 321: MonoDelta KUDU_EXPORT operator -(const MonoTime& t0, const MonoTime& 
t1);
The Google style guide frowns on operator overloading when it's incomplete (in 
this case, overloading operator- without also overloading operator+). Of 
course, operator+ for two MonoTimes doesn't make any sense, so perhaps we 
should drop this overload?


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


-- 
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: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to