Adar Dembo has posted comments on this change.
Change subject: [util/monotime] added handy operators for MonoTime
Patch Set 1:
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.
Line 211: MonoTime m_end = start.AddDelta(MonoDelta(-delta.nano_delta_));
Use ToNanoseconds() to avoid test friendship?
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
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.
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&
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-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins