From a196b9840f8f80c96306525b29a360d45eb6de32 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 5 Jan 2024 15:01:47 -0800 Subject: [PATCH] Run clang-tidy on headers (#3572) This patches bazel_clang_tidy handling of headers. I found an equivalent change at https://github.com/erenon/bazel_clang_tidy/pull/13, but that was [already rejected](https://github.com/erenon/bazel_clang_tidy/pull/13#issuecomment-1047007424). Per the criticism, this will result in redundant processing of headers. The project instead uses `HeaderFilterRegex: ".*"`, but that results in two problems: 1. When running with `-k`, errors are repeated when a header is included more than once, which is common. 2. clang-tidy including errors from headers that are included from other modules (e.g., abseil-cpp); filtering correctly is difficult. Given the trade-offs and options (including forking), I thought patching was preferable so long as it remains narrow. --- .pre-commit-config.yaml | 1 + MODULE.bazel | 2 ++ MODULE.bazel.lock | 28 +++++++++++++------ bazel/bazel_clang_tidy/0001_Add_hdrs.patch | 25 +++++++++++++++++ bazel/bazel_clang_tidy/BUILD | 9 ++++++ common/all_llvm_targets.cpp | 3 +- common/init_llvm.cpp | 2 +- common/init_llvm.h | 7 +++-- explorer/base/BUILD | 3 ++ toolchain/check/node_stack.h | 16 +++++------ toolchain/parse/node_ids.h | 21 ++++++++++---- toolchain/parse/tree.h | 3 +- .../parse/tree_node_location_translator.h | 2 ++ toolchain/sem_ir/file.h | 2 +- toolchain/sem_ir/inst.h | 11 +++++--- 15 files changed, 104 insertions(+), 31 deletions(-) create mode 100644 bazel/bazel_clang_tidy/0001_Add_hdrs.patch create mode 100644 bazel/bazel_clang_tidy/BUILD diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 868cee5abc7d..1b211dec4843 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -232,6 +232,7 @@ repos: exclude: | (?x)^( MODULE.bazel.lock| + bazel/bazel_clang_tidy/.*\.patch| bazel/llvm_project/.*\.patch| third_party/examples/.*/carbon/.*| )$ diff --git a/MODULE.bazel b/MODULE.bazel index 177e6859a9c0..97bff036ec08 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -67,6 +67,8 @@ clang_tidy_version = "d2aecc583d14c9554febeab185833c1e8cce5384" http_archive( name = "bazel_clang_tidy", + patch_args = ["-p1"], + patches = ["@carbon//bazel/bazel_clang_tidy:0001_Add_hdrs.patch"], sha256 = "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", strip_prefix = "bazel_clang_tidy-{0}".format(clang_tidy_version), urls = ["https://github.com/erenon/bazel_clang_tidy/archive/{0}.tar.gz".format(clang_tidy_version)], diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index d9ff9cf01049..252866bf36ae 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -1,6 +1,6 @@ { "lockFileVersion": 3, - "moduleFileHash": "1eff193166261836db74ff270f9bc0ef7d800107c1593383d78cd652d685bb6d", + "moduleFileHash": "5704ebd75cf3cb5f4ab56ba2939bb74d98752cd7eed0ea9357a9ed39ca41640e", "flags": { "cmdRegistries": [ "https://bcr.bazel.build/" @@ -63,6 +63,12 @@ { "tagName": "@bazel_tools//tools/build_defs/repo:http.bzl%http_archive", "attributeValues": { + "patch_args": [ + "-p1" + ], + "patches": [ + "@carbon//bazel/bazel_clang_tidy:0001_Add_hdrs.patch" + ], "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384", "urls": [ @@ -99,7 +105,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 106, + "line": 108, "column": 13 } } @@ -113,7 +119,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 75, + "line": 77, "column": 35 }, "imports": { @@ -130,7 +136,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 121, + "line": 123, "column": 29 }, "imports": { @@ -147,7 +153,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 133, + "line": 135, "column": 23 }, "imports": { @@ -163,7 +169,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 134, + "line": 136, "column": 17 } } @@ -177,7 +183,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 140, + "line": 142, "column": 20 }, "imports": { @@ -195,7 +201,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 141, + "line": 143, "column": 10 } } @@ -1673,6 +1679,12 @@ "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", "attributes": { + "patch_args": [ + "-p1" + ], + "patches": [ + "@@//bazel/bazel_clang_tidy:0001_Add_hdrs.patch" + ], "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384", "urls": [ diff --git a/bazel/bazel_clang_tidy/0001_Add_hdrs.patch b/bazel/bazel_clang_tidy/0001_Add_hdrs.patch new file mode 100644 index 000000000000..9940c8e2a111 --- /dev/null +++ b/bazel/bazel_clang_tidy/0001_Add_hdrs.patch @@ -0,0 +1,25 @@ +From c2b1cfb7ec8ba667218382b97a986a3ef2f7ff56 Mon Sep 17 00:00:00 2001 +From: jonmeow +Date: Fri, 5 Jan 2024 10:35:19 -0800 +Subject: [PATCH] Add hdrs so that clang-tidy runs on them directly. + +--- + clang_tidy/clang_tidy.bzl | 3 +++ + 1 file changed, 3 insertions(+) + +diff --git a/clang_tidy/clang_tidy.bzl b/clang_tidy/clang_tidy.bzl +index 4d515fc..1af7ee4 100644 +--- a/clang_tidy/clang_tidy.bzl ++++ b/clang_tidy/clang_tidy.bzl +@@ -94,6 +94,9 @@ def _rule_sources(ctx): + if hasattr(ctx.rule.attr, "srcs"): + for src in ctx.rule.attr.srcs: + srcs += [src for src in src.files.to_list() if src.is_source and check_valid_file_type(src)] ++ if hasattr(ctx.rule.attr, "hdrs"): ++ for src in ctx.rule.attr.hdrs: ++ srcs += [src for src in src.files.to_list() if src.is_source and check_valid_file_type(src)] + return srcs + + def _toolchain_flags(ctx, action_name = ACTION_NAMES.cpp_compile): +-- +2.43.0.472.g3155946c3a-goog diff --git a/bazel/bazel_clang_tidy/BUILD b/bazel/bazel_clang_tidy/BUILD new file mode 100644 index 000000000000..8fe7e2c41437 --- /dev/null +++ b/bazel/bazel_clang_tidy/BUILD @@ -0,0 +1,9 @@ +# Part of the Carbon Language project, under the Apache License v2.0 with LLVM +# Exceptions. See /LICENSE for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +package(default_visibility = ["//visibility:public"]) + +exports_files(glob([ + "*.patch", +])) diff --git a/common/all_llvm_targets.cpp b/common/all_llvm_targets.cpp index 039b0567868d..c5da2924a252 100644 --- a/common/all_llvm_targets.cpp +++ b/common/all_llvm_targets.cpp @@ -17,6 +17,7 @@ static auto InitLLVMTargets() -> void { // On program startup, set `InitLLVM::InitializeTargets` to be our // initialization function so that `InitLLVM` can call it at the right moment. -char InitLLVM::RegisterTargets = (InitializeTargets = &InitLLVMTargets, 0); +const char InitLLVM::RegisterTargets = + (InitializeTargets = &InitLLVMTargets, 0); } // namespace Carbon diff --git a/common/init_llvm.cpp b/common/init_llvm.cpp index 36cbde464414..4d798bdde954 100644 --- a/common/init_llvm.cpp +++ b/common/init_llvm.cpp @@ -34,6 +34,6 @@ InitLLVM::InitLLVM(int& argc, char**& argv) } } -auto (*InitLLVM::InitializeTargets)() -> void = nullptr; +InitLLVM::InitializeTargetsFn* InitLLVM::InitializeTargets = nullptr; } // namespace Carbon diff --git a/common/init_llvm.h b/common/init_llvm.h index d4995db7610d..b90ef4678287 100644 --- a/common/init_llvm.h +++ b/common/init_llvm.h @@ -24,17 +24,20 @@ class InitLLVM { ~InitLLVM() = default; private: + using InitializeTargetsFn = auto() -> void; + llvm::InitLLVM init_llvm_; llvm::SmallVector args_; // A pointer to the LLVM target initialization function, if :all_llvm_targets // is linked in. Otherwise nullptr. - static auto (*InitializeTargets)() -> void; + // NOLINTNEXTLINE(readability-identifier-naming): Constant after static init. + static InitializeTargetsFn* InitializeTargets; // The initializer of this static data member populates `InitializeTargets`. // Defined only if :all_llvm_targets is linked in. This is a member so that // it has access to `InitializeTargets`. - static char RegisterTargets; + static const char RegisterTargets; }; } // namespace Carbon diff --git a/explorer/base/BUILD b/explorer/base/BUILD index 85886dcaf4a1..a4241d3a49af 100644 --- a/explorer/base/BUILD +++ b/explorer/base/BUILD @@ -9,6 +9,9 @@ package(default_visibility = ["//explorer:__subpackages__"]) cc_library( name = "arena", hdrs = ["arena.h"], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":nonnull", "@llvm-project//llvm:Support", diff --git a/toolchain/check/node_stack.h b/toolchain/check/node_stack.h index 2e25b309ccfd..25fe469d1f7b 100644 --- a/toolchain/check/node_stack.h +++ b/toolchain/check/node_stack.h @@ -150,7 +150,7 @@ class NodeStack { template auto PopWithParseNode() -> auto { constexpr IdKind RequiredIdKind = ParseNodeKindToIdKind(RequiredParseKind); - auto NodeIdCast = [&](auto back) { + auto node_id_cast = [&](auto back) { using NodeIdT = Parse::NodeIdForKind; return std::pair(back); }; @@ -158,37 +158,37 @@ class NodeStack { if constexpr (RequiredIdKind == IdKind::InstId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::InstBlockId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::FunctionId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::ClassId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::InterfaceId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::NameId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } if constexpr (RequiredIdKind == IdKind::TypeId) { auto back = PopWithParseNode(); RequireParseKind(back.first); - return NodeIdCast(back); + return node_id_cast(back); } CARBON_FATAL() << "Unpoppable IdKind for parse kind: " << RequiredParseKind << "; see value in ParseNodeKindToIdKind"; diff --git a/toolchain/parse/node_ids.h b/toolchain/parse/node_ids.h index 84556b683cbe..ea47fff75493 100644 --- a/toolchain/parse/node_ids.h +++ b/toolchain/parse/node_ids.h @@ -23,7 +23,8 @@ struct NodeId : public IdBase { static constexpr InvalidNodeId Invalid; using IdBase::IdBase; - constexpr NodeId(InvalidNodeId) : IdBase(NodeId::InvalidIndex) {} + // NOLINTNEXTLINE(google-explicit-constructor) + constexpr NodeId(InvalidNodeId /*invalid*/) : IdBase(NodeId::InvalidIndex) {} }; // For looking up the type associated with a given id type. @@ -34,9 +35,12 @@ struct NodeForId; // ``: template struct NodeIdForKind : public NodeId { + // NOLINTNEXTLINE(readability-identifier-naming) static const NodeKind& Kind; constexpr explicit NodeIdForKind(NodeId node_id) : NodeId(node_id) {} - constexpr NodeIdForKind(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {} + // NOLINTNEXTLINE(google-explicit-constructor) + constexpr NodeIdForKind(InvalidNodeId /*invalid*/) + : NodeId(NodeId::InvalidIndex) {} }; template const NodeKind& NodeIdForKind::Kind = K; @@ -52,7 +56,9 @@ struct NodeIdInCategory : public NodeId { // overlaps with `Category`. constexpr explicit NodeIdInCategory(NodeId node_id) : NodeId(node_id) {} - constexpr NodeIdInCategory(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {} + // NOLINTNEXTLINE(google-explicit-constructor) + constexpr NodeIdInCategory(InvalidNodeId /*invalid*/) + : NodeId(NodeId::InvalidIndex) {} }; // Aliases for `NodeIdInCategory` to describe particular categories of nodes. @@ -69,10 +75,13 @@ template struct NodeIdOneOf : public NodeId { constexpr explicit NodeIdOneOf(NodeId node_id) : NodeId(node_id) {} template + // NOLINTNEXTLINE(google-explicit-constructor) NodeIdOneOf(NodeIdForKind node_id) : NodeId(node_id) { static_assert(T::Kind == Kind || U::Kind == Kind); } - constexpr NodeIdOneOf(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {} + // NOLINTNEXTLINE(google-explicit-constructor) + constexpr NodeIdOneOf(InvalidNodeId /*invalid*/) + : NodeId(NodeId::InvalidIndex) {} }; using AnyClassDeclId = NodeIdOneOf; @@ -85,7 +94,9 @@ using AnyInterfaceDeclId = template struct NodeIdNot : public NodeId { constexpr explicit NodeIdNot(NodeId node_id) : NodeId(node_id) {} - constexpr NodeIdNot(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {} + // NOLINTNEXTLINE(google-explicit-constructor) + constexpr NodeIdNot(InvalidNodeId /*invalid*/) + : NodeId(NodeId::InvalidIndex) {} }; // Note that the support for extracting these types using the `Tree::Extract*` diff --git a/toolchain/parse/tree.h b/toolchain/parse/tree.h index 86e519eaff20..8cf75a86c9d4 100644 --- a/toolchain/parse/tree.h +++ b/toolchain/parse/tree.h @@ -439,7 +439,8 @@ auto Tree::ExtractNodeFromChildren( // On error try again, this time capturing a trace. ErrorBuilder trace; TryExtractNodeFromChildren(children, &trace); - CARBON_FATAL() << "Malformed parse node:\n" << Error(trace).message(); + CARBON_FATAL() << "Malformed parse node:\n" + << static_cast(trace).message(); } return *result; } diff --git a/toolchain/parse/tree_node_location_translator.h b/toolchain/parse/tree_node_location_translator.h index 0823b9c72cf0..d9444538d5e9 100644 --- a/toolchain/parse/tree_node_location_translator.h +++ b/toolchain/parse/tree_node_location_translator.h @@ -13,11 +13,13 @@ namespace Carbon::Parse { class NodeLocation { public: + // NOLINTNEXTLINE(google-explicit-constructor) NodeLocation(NodeId node_id) : NodeLocation(node_id, false) {} NodeLocation(NodeId node_id, bool token_only) : node_id_(node_id), token_only_(token_only) {} // TODO: Have some other way of representing diagnostic that applies to a file // as a whole. + // NOLINTNEXTLINE(google-explicit-constructor) NodeLocation(InvalidNodeId node_id) : NodeLocation(node_id, false) {} auto node_id() const -> NodeId { return node_id_; } diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index ec1221e89f6d..26485071a3b5 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -178,7 +178,7 @@ class File : public Printable { const File* builtins); File(const File&) = delete; - File& operator=(const File&) = delete; + auto operator=(const File&) -> File& = delete; // Verifies that invariants of the semantics IR hold. auto Verify() const -> ErrorOr; diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 0aa7274881b4..4e09a97d8cf3 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -57,12 +57,14 @@ struct InstLikeTypeInfoBase { // A particular type of instruction is instruction-like. template struct InstLikeTypeInfo< - TypedInst, - (bool)std::is_same_v> + TypedInst, static_cast(std::is_same_v)> : InstLikeTypeInfoBase { static_assert(!HasKindMemberAsField, "Instruction type should not have a kind field"); - static auto GetKind(TypedInst) -> InstKind { return TypedInst::Kind; } + static auto GetKind(TypedInst /*inst*/) -> InstKind { + return TypedInst::Kind; + } static auto IsKind(InstKind kind) -> bool { return kind == TypedInst::Kind; } // A name that can be streamed to an llvm::raw_ostream. static auto DebugName() -> InstKind { return TypedInst::Kind; } @@ -71,7 +73,8 @@ struct InstLikeTypeInfo< // An instruction category is instruction-like. template struct InstLikeTypeInfo< - InstCat, (bool)std::is_same_v> + InstCat, static_cast( + std::is_same_v)> : InstLikeTypeInfoBase { static_assert(HasKindMemberAsField, "Instruction category should have a kind field");