kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ale...@apache.org
Subject [kudu] 01/02: clang-tidy: no warnings on structs with all public data members
Date Thu, 19 Dec 2019 23:09:58 GMT
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 <adar@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 <awong@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'


Mime
View raw message