You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2019/12/19 23:09:57 UTC
[kudu] branch master updated (b0b014b -> 643bf32)
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from b0b014b cmake: prettier messages for AVX2 support
new 684725f clang-tidy: no warnings on structs with all public data members
new 643bf32 [monotime] deprecating MonoTime::GetDeltaSince()
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
build-support/clang_tidy_gerrit.py | 29 ++--------------
src/kudu/.clang-tidy | 50 +++++++++++++++++++++++++++-
src/kudu/rpc/result_tracker.cc | 3 +-
src/kudu/rpc/rpc-test-base.h | 2 +-
src/kudu/tserver/tablet_copy_service-test.cc | 2 +-
src/kudu/tserver/tablet_service.cc | 4 +--
src/kudu/util/monotime-test.cc | 14 +++-----
src/kudu/util/monotime.cc | 12 +++----
src/kudu/util/monotime.h | 12 +++++--
9 files changed, 75 insertions(+), 53 deletions(-)
[kudu] 01/02: clang-tidy: no warnings on structs with all public
data members
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 684725fd457e308ee71889d92d8346dd081d4e5a
Author: Adar Dembo <ad...@cloudera.com>
AuthorDate: Wed Dec 18 21:53:32 2019 -0800
clang-tidy: no warnings on structs with all public data members
Since the LLVM 9 upgrade clang-tidy has been warning on things like this:
src/kudu/tserver/tablet_copy_source_session.h:66:11: warning: member variable 'size' has public visibility [misc-non-private-member-variables-in-classes]
int64_t size;
^
'size' is indeed a public data member, but as per the Google Style Guide[1],
structs should consist of nothing but public data members.
I couldn't find a way to configure clang-tidy correctly on this issue, so
instead I reconfigured the corresponding check to not warn about any struct
or class where all data members are public. In the process, I rediscovered
our existing .clang-tidy configuration file and moved the recently disabled
checks there.
1. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
Change-Id: Ibf9913fd0b0410b44cf36f044256ef0266949d95
Reviewed-on: http://gerrit.cloudera.org:8080/14930
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
build-support/clang_tidy_gerrit.py | 29 ++--------------------
src/kudu/.clang-tidy | 50 +++++++++++++++++++++++++++++++++++++-
2 files changed, 51 insertions(+), 28 deletions(-)
diff --git a/build-support/clang_tidy_gerrit.py b/build-support/clang_tidy_gerrit.py
index cd500b1..0419bf6 100755
--- a/build-support/clang_tidy_gerrit.py
+++ b/build-support/clang_tidy_gerrit.py
@@ -45,31 +45,6 @@ GERRIT_PASSWORD = os.getenv('TIDYBOT_PASSWORD')
GERRIT_URL = "https://gerrit.cloudera.org"
-DISABLED_TIDY_CHECKS=[
- # Although useful in production code, we use magic numbers liberally in
- # tests, and many would be net less clear as named constants.
- 'readability-magic-numbers',
-
- # IWYU has specific rules for ordering '-inl.h' include files, i.e.
- # 'header.h' and its 'header-inl.h' counterpart. It seems in some cases
- # including 'header-inl.h' before 'header.h' might even lead to compilation
- # failures. So, IWYU intentionally re-orders them even if 'header-inl.h'
- # comes before 'header.h' lexicographically in default C locale:
- # https://github.com/apache/kudu/blob/ \
- # 89ce529e945731c48445db4a6f8af11f9f905aab/build-support/iwyu/ \
- # fix_includes.py#L1786-L1787
- #
- # That ordering contradicts with what clang-tidy recommends when using the
- # 'llvm-include-order' check. To avoid confusion, let's disable the
- # 'llvm-include-order'.
- #
- # TODO(aserbin): clarify whether it's possible to customize clang-tidy
- # behavior w.r.t. the sorting of such header files using
- # the format style options described at
- # https://clang.llvm.org/docs/ClangFormatStyleOptions.html
- 'llvm-include-order',
-]
-
def run_tidy(sha="HEAD", is_rev_range=False):
diff_cmdline = ["git", "diff" if is_rev_range else "show", sha]
@@ -79,6 +54,8 @@ def run_tidy(sha="HEAD", is_rev_range=False):
# Produce a separate diff for each file and run clang-tidy-diff on it
# in parallel.
+ #
+ # Note: this will incorporate any configuration from .clang-tidy.
def tidy_on_path(path):
patch_file = tempfile.NamedTemporaryFile()
cmd = diff_cmdline + [
@@ -87,11 +64,9 @@ def run_tidy(sha="HEAD", is_rev_range=False):
"--",
path]
subprocess.check_call(cmd, stdout=patch_file, cwd=ROOT)
- checks_arg_value = ",".join([ "-" + c for c in DISABLED_TIDY_CHECKS ])
cmdline = [CLANG_TIDY_DIFF,
"-clang-tidy-binary", CLANG_TIDY,
"-p0",
- "-checks=" + checks_arg_value,
"--",
"-DCLANG_TIDY"] + compile_flags.get_flags()
return subprocess.check_output(
diff --git a/src/kudu/.clang-tidy b/src/kudu/.clang-tidy
index e4af41b..1b6c4ea 100644
--- a/src/kudu/.clang-tidy
+++ b/src/kudu/.clang-tidy
@@ -14,7 +14,49 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-Checks: 'clang-diagnostic-*,clang-analyzer-*,-clang-analyzer-alpha*,-*,readability-*,-readability-braces-around-statements,llvm-include-order,misc-*,-modernize-*,performance-*,-readability-implicit-bool-conversion,google-*,-google-readability-braces-around-statements,-readability-redundant-string-init,modernize-make-shared,modernize-shrink-to-fit,modernize-use-emplace,modernize-pass-by-value,modernize-use-nullptr,bugprone-*,clang-analyzer-deadcode.DeadStores,hicpp-noexcept-move'
+
+Checks: >-
+ -*,
+ bugprone-*,
+ clang-analyzer-*,
+ clang-analyzer-deadcode.DeadStores,
+ -clang-analyzer-alpha*,
+ clang-diagnostic-*,
+ google-*,
+ -google-readability-braces-around-statements,
+ hicpp-noexcept-move,
+ # IWYU has specific rules for ordering '-inl.h' include files, i.e.
+ # 'header.h' and its 'header-inl.h' counterpart. It seems in some cases
+ # including 'header-inl.h' before 'header.h' might even lead to compilation
+ # failures. So, IWYU intentionally re-orders them even if 'header-inl.h'
+ # comes before 'header.h' lexicographically in default C locale:
+ # https://github.com/apache/kudu/blob/ \
+ # 89ce529e945731c48445db4a6f8af11f9f905aab/build-support/iwyu/ \
+ # fix_includes.py#L1786-L1787
+ #
+ # That ordering contradicts with what clang-tidy recommends when using the
+ # 'llvm-include-order' check.
+ #
+ # TODO(aserbin): clarify whether it's possible to customize clang-tidy
+ # behavior w.r.t. the sorting of such header files using
+ # the format style options described at
+ # https://clang.llvm.org/docs/ClangFormatStyleOptions.html
+ -llvm-include-order,
+ misc-*,
+ -modernize-*,
+ modernize-make-shared,
+ modernize-pass-by-value,
+ modernize-shrink-to-fit,
+ modernize-use-emplace,
+ modernize-use-nullptr,
+ performance-*,
+ readability-*,
+ -readability-braces-around-statements,
+ -readability-implicit-bool-conversion,
+ # Although useful in production code, we use magic numbers liberally in
+ # tests, and many would be net less clear as named constants.
+ -readability-magic-numbers,
+ -readability-redundant-string-init
HeaderFilterRegex: '.*,-*.pb.h'
CheckOptions:
- key: google-runtime-references.WhiteListTypes
@@ -35,3 +77,9 @@ CheckOptions:
value: CamelCase
- key: readability-identifier-naming.GlobalConstantPrefix
value: 'k'
+ # The Google Style Guide allows for structs to contain public data members.
+ # Unfortunately there doesn't seem to be a way to configure clang-tidy to
+ # permit this, so we allow classes/structs where all data members are public
+ # as a proxy.
+ - key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
+ value: '1'
[kudu] 02/02: [monotime] deprecating MonoTime::GetDeltaSince()
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 643bf32bb82e6f08c02cae94d0dab5cac9d61216
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Wed Dec 18 00:25:35 2019 -0800
[monotime] deprecating MonoTime::GetDeltaSince()
I find that
MonoTime t0 = ...;
MonoTime t1 = ...;
MonoDelta delta = t1 - t0;
is easier to write, read, and understand than
MonoTime t0 = ...;
MonoTime t1 = ...;
MonoDelta delta = t1.GetDeltaSince(t0);
So, I replaced usage of the MonoTime::GetDeltaSince() with
the operator and marked the former method as deprecated.
This patch doesn't contain any functional modifications.
Change-Id: I9611c8a20e02b117757842bc3b46b8faf82d9700
Reviewed-on: http://gerrit.cloudera.org:8080/14932
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Yingchun Lai <40...@qq.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
src/kudu/rpc/result_tracker.cc | 3 +--
src/kudu/rpc/rpc-test-base.h | 2 +-
src/kudu/tserver/tablet_copy_service-test.cc | 2 +-
src/kudu/tserver/tablet_service.cc | 4 +---
src/kudu/util/monotime-test.cc | 14 +++++---------
src/kudu/util/monotime.cc | 12 ++++++------
src/kudu/util/monotime.h | 12 +++++++++---
7 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/src/kudu/rpc/result_tracker.cc b/src/kudu/rpc/result_tracker.cc
index d26ff87..f8f5c46 100644
--- a/src/kudu/rpc/result_tracker.cc
+++ b/src/kudu/rpc/result_tracker.cc
@@ -556,8 +556,7 @@ void ResultTracker::ClientState::GCCompletionRecords(
}
string ResultTracker::ClientState::ToString() const {
- auto since_last_heard =
- MonoTime::Now().GetDeltaSince(last_heard_from);
+ auto since_last_heard = MonoTime::Now() - last_heard_from;
string result = Substitute("Client State[Last heard from: $0s ago, "
"$1 CompletionRecords:",
since_last_heard.ToString(),
diff --git a/src/kudu/rpc/rpc-test-base.h b/src/kudu/rpc/rpc-test-base.h
index 2fb742e..e26bc9d 100644
--- a/src/kudu/rpc/rpc-test-base.h
+++ b/src/kudu/rpc/rpc-test-base.h
@@ -212,7 +212,7 @@ class GenericCalculatorService : public ServiceIf {
LOG(INFO) << "got call: " << pb_util::SecureShortDebugString(req);
SleepFor(MonoDelta::FromMicroseconds(req.sleep_micros()));
- MonoDelta duration(MonoTime::Now().GetDeltaSince(incoming->GetTimeReceived()));
+ MonoDelta duration(MonoTime::Now() - incoming->GetTimeReceived());
CHECK_GE(duration.ToMicroseconds(), req.sleep_micros());
SleepResponsePB resp;
incoming->RespondSuccess(resp);
diff --git a/src/kudu/tserver/tablet_copy_service-test.cc b/src/kudu/tserver/tablet_copy_service-test.cc
index 1dc0db6..25f076c 100644
--- a/src/kudu/tserver/tablet_copy_service-test.cc
+++ b/src/kudu/tserver/tablet_copy_service-test.cc
@@ -510,7 +510,7 @@ TEST_F(TabletCopyServiceTest, TestSessionTimeout) {
break;
}
SleepFor(MonoDelta::FromMilliseconds(1)); // 1 ms
- } while (MonoTime::Now().GetDeltaSince(start_time).ToSeconds() < 10);
+ } while ((MonoTime::Now() - start_time).ToSeconds() < 10);
ASSERT_FALSE(resp.session_is_active()) << "Tablet Copy session did not time out!";
}
diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc
index 2839d42..97a4cdd 100644
--- a/src/kudu/tserver/tablet_service.cc
+++ b/src/kudu/tserver/tablet_service.cc
@@ -31,7 +31,6 @@
#include <boost/optional/optional.hpp>
#include <gflags/gflags.h>
-#include <gflags/gflags_declare.h>
#include <glog/logging.h>
#include "kudu/clock/clock.h"
@@ -91,7 +90,6 @@
#include "kudu/tserver/tserver_admin.pb.h"
#include "kudu/tserver/tserver_service.pb.h"
#include "kudu/util/auto_release_pool.h"
-#include "kudu/util/bitset.h"
#include "kudu/util/crc.h"
#include "kudu/util/debug/trace_event.h"
#include "kudu/util/faststring.h"
@@ -2713,7 +2711,7 @@ namespace {
// Helper to clamp a client deadline for a scan to the max supported by the server.
MonoTime ClampScanDeadlineForWait(const MonoTime& deadline, bool* was_clamped) {
MonoTime now = MonoTime::Now();
- if (deadline.GetDeltaSince(now).ToMilliseconds() > FLAGS_scanner_max_wait_ms) {
+ if ((deadline - now).ToMilliseconds() > FLAGS_scanner_max_wait_ms) {
*was_clamped = true;
return now + MonoDelta::FromMilliseconds(FLAGS_scanner_max_wait_ms);
}
diff --git a/src/kudu/util/monotime-test.cc b/src/kudu/util/monotime-test.cc
index 755dac5..0fba667 100644
--- a/src/kudu/util/monotime-test.cc
+++ b/src/kudu/util/monotime-test.cc
@@ -106,7 +106,7 @@ TEST(TestMonoTime, TestTimeSpec) {
ts.tv_sec = 1;
ts.tv_nsec = 1;
MonoTime one_sec_one_nano_actual(ts);
- ASSERT_EQ(0, one_sec_one_nano_expected.GetDeltaSince(one_sec_one_nano_actual).ToNanoseconds());
+ ASSERT_EQ(0, (one_sec_one_nano_expected - one_sec_one_nano_actual).ToNanoseconds());
MonoDelta zero_sec_two_nanos(MonoDelta::FromNanoseconds(2L));
zero_sec_two_nanos.ToTimeSpec(&ts);
@@ -127,11 +127,9 @@ TEST(TestMonoTime, TestDeltas) {
alarm(360);
const MonoDelta max_delta(MonoDelta::FromSeconds(0.1));
MonoTime prev(MonoTime::Now());
- MonoTime next;
MonoDelta cur_delta;
do {
- next = MonoTime::Now();
- cur_delta = next.GetDeltaSince(prev);
+ cur_delta = MonoTime::Now() - prev;
} while (cur_delta.LessThan(max_delta));
alarm(0);
}
@@ -153,11 +151,9 @@ static void DoTestMonoTimePerf() {
const MonoDelta max_delta(MonoDelta::FromMilliseconds(500));
uint64_t num_calls = 0;
MonoTime prev(MonoTime::Now());
- MonoTime next;
MonoDelta cur_delta;
do {
- next = MonoTime::Now();
- cur_delta = next.GetDeltaSince(prev);
+ cur_delta = MonoTime::Now() - prev;
num_calls++;
} while (cur_delta.LessThan(max_delta));
LOG(INFO) << "DoTestMonoTimePerf():"
@@ -170,7 +166,7 @@ TEST(TestMonoTime, TestSleepFor) {
MonoDelta sleep = MonoDelta::FromMilliseconds(100);
SleepFor(sleep);
MonoTime end = MonoTime::Now();
- MonoDelta actualSleep = end.GetDeltaSince(start);
+ MonoDelta actualSleep = end - start;
ASSERT_GE(actualSleep.ToNanoseconds(), sleep.ToNanoseconds());
}
@@ -186,7 +182,7 @@ TEST(TestMonoTime, TestSleepForOverflow) {
MonoDelta sleep = MonoDelta::FromNanoseconds(1L << 32);
SleepFor(sleep);
MonoTime end = MonoTime::Now();
- MonoDelta actualSleep = end.GetDeltaSince(start);
+ MonoDelta actualSleep = end - start;
ASSERT_GE(actualSleep.ToNanoseconds(), sleep.ToNanoseconds());
}
diff --git a/src/kudu/util/monotime.cc b/src/kudu/util/monotime.cc
index 89c795d..b6ffa65 100644
--- a/src/kudu/util/monotime.cc
+++ b/src/kudu/util/monotime.cc
@@ -201,11 +201,7 @@ bool MonoTime::Initialized() const {
}
MonoDelta MonoTime::GetDeltaSince(const MonoTime &rhs) const {
- DCHECK(Initialized());
- DCHECK(rhs.Initialized());
- int64_t delta(nanos_);
- delta -= rhs.nanos_;
- return MonoDelta(delta);
+ return rhs - *this;
}
void MonoTime::AddDelta(const MonoDelta &delta) {
@@ -328,7 +324,11 @@ MonoTime operator-(const MonoTime& t, const MonoDelta& delta) {
}
MonoDelta operator-(const MonoTime& t_end, const MonoTime& t_beg) {
- return t_end.GetDeltaSince(t_beg);
+ DCHECK(t_beg.Initialized());
+ DCHECK(t_end.Initialized());
+ int64_t delta(t_end.nanos_);
+ delta -= t_beg.nanos_;
+ return MonoDelta(delta);
}
} // namespace kudu
diff --git a/src/kudu/util/monotime.h b/src/kudu/util/monotime.h
index bb8ec35..f0013a1 100644
--- a/src/kudu/util/monotime.h
+++ b/src/kudu/util/monotime.h
@@ -131,6 +131,7 @@ class KUDU_EXPORT MonoDelta {
static const int64_t kUninitialized;
friend class MonoTime;
+ friend MonoDelta operator-(const class MonoTime&, const class MonoTime&);
FRIEND_TEST(TestMonoTime, TestDeltaConversions);
explicit MonoDelta(int64_t delta);
@@ -169,6 +170,8 @@ class KUDU_EXPORT MonoTime {
/// Select the earliest between the specified time points.
///
+ /// @deprecated Use @c use std::min() instead.
+ ///
/// @param [in] a
/// The first MonoTime object to select from.
/// @param [in] b
@@ -187,11 +190,15 @@ class KUDU_EXPORT MonoTime {
/// Compute time interval between the point in time specified by this
/// and the specified object.
///
+ /// @deprecated Use @c kudu::operator-(const MonoTime&, const MonoTime&)
+ /// instead.
+ ///
/// @param [in] rhs
/// The object that corresponds to the left boundary of the time interval,
/// where this object corresponds to the right boundary of the interval.
/// @return The resulting time interval represented as a MonoDelta object.
- MonoDelta GetDeltaSince(const MonoTime &rhs) const;
+ MonoDelta GetDeltaSince(const MonoTime &rhs) const ATTRIBUTE_DEPRECATED(
+ "use kudu::operator-(const MonoTime&, const MonoTime&) instead");
/// Advance this object's time specification by the specified interval.
///
@@ -246,6 +253,7 @@ class KUDU_EXPORT MonoTime {
private:
friend class MonoDelta;
+ friend MonoDelta operator-(const MonoTime&, const MonoTime&);
FRIEND_TEST(TestMonoTime, TestTimeSpec);
FRIEND_TEST(TestMonoTime, TestDeltaConversions);
@@ -403,8 +411,6 @@ MonoTime KUDU_EXPORT operator-(const MonoTime& t, const MonoDelta& delta);
/// Compute the time interval between the specified points in time.
///
-/// Semantically, this is equivalent to t0.GetDeltaSince(t1).
-///
/// @param [in] t_end
/// The second point in time. Semantically corresponds to the end
/// of the resulting time interval.