From 7fe8bb308bb37a4366cca650a6083cc055464c75 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 1 Mar 2023 13:12:51 -0800 Subject: [PATCH] Clean up clang-tidy issues in explorer. (#2621) google-readability-function-size and readability-function-size were _both_ triggering on TypeCheckExp. It looks like the Google version may be a subset of the general version, so I've disabled the Google version while keeping the general version and adding a NOLINT for it. I manually removed the `const` in cases like `Nonnull` based on the readability-const-return-type warning. i.e., where a return type is a pointer, the `const` isn't meaningful and the tidy check was warning about that. Added a NOLINT for misc-definitions-in-headers on IsRecursivelyTransformable. I think that's the right choice for the `constexpr`, the warning didn't feel accurate and may be getting confused by the templating. I changed the structure of `carbon_files` in the fuzzer because the `new` was causing a warning about exceptions. However, also disabling bugprone-exception-escape because it's what was flagging this and it's not really a helpful warning. Other changes were automated. Co-authored-by: Richard Smith --- .clang-tidy | 6 +++++- explorer/ast/declaration.cpp | 2 +- explorer/fuzzing/ast_to_proto.cpp | 2 +- explorer/fuzzing/ast_to_proto_test.cpp | 4 ++-- explorer/fuzzing/proto_to_carbon_test.cpp | 5 +++-- explorer/interpreter/matching_impl_set.cpp | 18 +++++++++--------- explorer/interpreter/matching_impl_set.h | 8 ++++---- explorer/interpreter/type_checker.cpp | 3 ++- explorer/interpreter/value.h | 2 +- explorer/interpreter/value_transform.h | 8 ++++---- 10 files changed, 32 insertions(+), 26 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 6429e4b7854d..5856dc790236 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -3,13 +3,17 @@ # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception --- +# - bugprone-exception-escape finds issues like out-of-memory in main(). We +# don't use exceptions, so it's unlikely to find real issues. +# - google-readability-function-size overlaps with readability-function-size. # - modernize-use-nodiscard is disabled because it only fixes const methods, # not non-const, which yields distracting results on accessors. # - performance-unnecessary-value-param is disabled because it duplicate # modernize-pass-by-value. Checks: -*, bugprone-*, -bugprone-branch-clone, -bugprone-easily-swappable-parameters, - -bugprone-narrowing-conversions, google-*, -google-readability-todo, + -bugprone-exception-escape, -bugprone-narrowing-conversions, google-*, + -google-readability-function-size, -google-readability-todo, misc-definitions-in-headers, misc-misplaced-const, misc-redundant-expression, misc-static-assert, misc-unconventional-assign-operator, misc-uniqueptr-reset-release, misc-unused-*, modernize-*, diff --git a/explorer/ast/declaration.cpp b/explorer/ast/declaration.cpp index c2437d6ba917..1caea54f192c 100644 --- a/explorer/ast/declaration.cpp +++ b/explorer/ast/declaration.cpp @@ -441,7 +441,7 @@ void AlternativeSignature::PrintID(llvm::raw_ostream& out) const { auto ChoiceDeclaration::FindAlternative(std::string_view name) const -> std::optional { - for (auto* alt : alternatives()) { + for (const auto* alt : alternatives()) { if (alt->name() == name) { return alt; } diff --git a/explorer/fuzzing/ast_to_proto.cpp b/explorer/fuzzing/ast_to_proto.cpp index 173b30ade370..0c3e2074981e 100644 --- a/explorer/fuzzing/ast_to_proto.cpp +++ b/explorer/fuzzing/ast_to_proto.cpp @@ -618,7 +618,7 @@ static auto DeclaredNameToProto(const DeclaredName& name) -> Fuzzing::DeclaredName { Fuzzing::DeclaredName name_proto; name_proto.set_name(std::string(name.inner_name())); - for (auto& [loc, qual] : name.qualifiers()) { + for (const auto& [loc, qual] : name.qualifiers()) { name_proto.add_qualifiers(qual); } return name_proto; diff --git a/explorer/fuzzing/ast_to_proto_test.cpp b/explorer/fuzzing/ast_to_proto_test.cpp index aa0bfb296cd4..e7994358df2d 100644 --- a/explorer/fuzzing/ast_to_proto_test.cpp +++ b/explorer/fuzzing/ast_to_proto_test.cpp @@ -124,7 +124,7 @@ TEST(AstToProtoTest, SetsAllProtoFields) { auto main(int argc, char** argv) -> int { ::testing::InitGoogleTest(&argc, argv); // gtest should remove flags, leaving just input files. - Carbon::Testing::carbon_files = - new std::vector(&argv[1], &argv[argc]); + std::vector carbon_files(&argv[1], &argv[argc]); + Carbon::Testing::carbon_files = &carbon_files; return RUN_ALL_TESTS(); } diff --git a/explorer/fuzzing/proto_to_carbon_test.cpp b/explorer/fuzzing/proto_to_carbon_test.cpp index 34808b3bcdc6..1ae05fe39e2e 100644 --- a/explorer/fuzzing/proto_to_carbon_test.cpp +++ b/explorer/fuzzing/proto_to_carbon_test.cpp @@ -57,7 +57,8 @@ TEST(ProtoToCarbonTest, Roundtrip) { auto main(int argc, char** argv) -> int { ::testing::InitGoogleTest(&argc, argv); - Carbon::Testing::carbon_files = - new std::vector(&argv[1], &argv[argc]); + // gtest should remove flags, leaving just input files. + std::vector carbon_files(&argv[1], &argv[argc]); + Carbon::Testing::carbon_files = &carbon_files; return RUN_ALL_TESTS(); } diff --git a/explorer/interpreter/matching_impl_set.cpp b/explorer/interpreter/matching_impl_set.cpp index 428ad05382c4..aa6e279a14d9 100644 --- a/explorer/interpreter/matching_impl_set.cpp +++ b/explorer/interpreter/matching_impl_set.cpp @@ -17,7 +17,7 @@ namespace Carbon { // and adds them to the signature of a `Match` object. class MatchingImplSet::LeafCollector { public: - LeafCollector(Match* match) : match_(match) {} + explicit LeafCollector(Match* match) : match_(match) {} void Collect(const Value* value) { value->Visit( @@ -28,15 +28,15 @@ class MatchingImplSet::LeafCollector { private: // Most kinds of value don't contribute to the signature. - void VisitValue(const Value*) {} + void VisitValue(const Value* /*unused*/) {} - void VisitValue(const TypeType*) { Collect(Label::TypeType); } + void VisitValue(const TypeType* /*unused*/) { Collect(Label::TypeType); } - void VisitValue(const BoolType*) { Collect(Label::BoolType); } + void VisitValue(const BoolType* /*unused*/) { Collect(Label::BoolType); } - void VisitValue(const IntType*) { Collect(Label::IntType); } + void VisitValue(const IntType* /*unused*/) { Collect(Label::IntType); } - void VisitValue(const StringType*) { Collect(Label::StringType); } + void VisitValue(const StringType* /*unused*/) { Collect(Label::StringType); } void VisitValue(const StaticArrayType* array) { Collect(Label::ArrayType); @@ -57,7 +57,7 @@ class MatchingImplSet::LeafCollector { void VisitValue(const TupleType* tuple_type) { Collect(Label::TupleType); - for (auto* elem_type : tuple_type->elements()) { + for (const auto* elem_type : tuple_type->elements()) { Collect(elem_type); } } @@ -92,7 +92,6 @@ class MatchingImplSet::LeafCollector { } } - private: Match* match_; }; @@ -100,7 +99,8 @@ auto MatchingImplSet::GetLabelForDeclaration(const Declaration& declaration) -> Label { auto [it, added] = declaration_labels_.insert( {&declaration, - Label(int(Label::FirstDeclarationLabel) + declaration_labels_.size())}); + static_cast