From f9014a6d100c92393cd122a62f4fd43d2ae56784 Mon Sep 17 00:00:00 2001 From: Jon Meow Date: Thu, 24 Mar 2022 13:22:44 -0700 Subject: [PATCH] clang-tidy with readability checks (#1148) --- .clang-tidy | 7 +++- common/check_internal.h | 4 +- executable_semantics/common/error.h | 21 ++++++---- executable_semantics/common/error_test.cpp | 2 - executable_semantics/interpreter/action.cpp | 2 +- .../interpreter/type_checker.cpp | 41 ++++++++++--------- .../interpreter/type_checker.h | 6 +-- executable_semantics/interpreter/value.cpp | 2 +- executable_semantics/interpreter/value.h | 2 - toolchain/common/yaml_test_helpers.cpp | 6 +-- toolchain/common/yaml_test_helpers.h | 6 ++- toolchain/diagnostics/null_diagnostics.h | 4 +- toolchain/lexer/numeric_literal.cpp | 5 ++- toolchain/lexer/tokenized_buffer.cpp | 2 +- toolchain/lexer/tokenized_buffer.h | 4 +- toolchain/lexer/tokenized_buffer_test.cpp | 2 +- toolchain/parser/parse_test_helpers.h | 6 ++- toolchain/parser/parser_impl.h | 2 +- 18 files changed, 68 insertions(+), 56 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index fb96cd2f60df..014499e5dcdc 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -16,7 +16,12 @@ Checks: -modernize-avoid-c-arrays, -modernize-return-braced-init-list, -modernize-use-default-member-init, -modernize-use-emplace, -modernize-use-nodiscard, performance-*, -performance-unnecessary-value-param, - readability-braces-around-statements, readability-identifier-naming + readability-*, -readability-convert-member-functions-to-static, + -readability-function-cognitive-complexity, -readability-else-after-return, + -readability-implicit-bool-conversion, -readability-magic-numbers, + -readability-make-member-function-const, + -readability-qualified-auto, -readability-static-definition-in-anonymous-namespace, + -readability-suspicious-call-argument, -readability-use-anyofallof WarningsAsErrors: true CheckOptions: - { key: readability-identifier-naming.ClassCase, value: CamelCase } diff --git a/common/check_internal.h b/common/check_internal.h index 1740fbbcc495..f8c7f51c149e 100644 --- a/common/check_internal.h +++ b/common/check_internal.h @@ -52,7 +52,7 @@ class ExitingStream { return *this; } - auto operator<<(AddSeparator /*unused*/) -> ExitingStream& { + auto operator<<(AddSeparator /*discarded*/) -> ExitingStream& { separator_ = true; return *this; } @@ -60,7 +60,7 @@ class ExitingStream { // Low-precedence binary operator overload used in check.h macros to flush the // output and exit the program. We do this in a binary operator rather than // the destructor to ensure good debug info and backtraces for errors. - [[noreturn]] friend auto operator|(Helper /*unused*/, ExitingStream& rhs) { + [[noreturn]] friend auto operator|(Helper /*discarded*/, ExitingStream& rhs) { // Finish with a newline. llvm::errs() << "\n"; if (rhs.treat_as_bug_) { diff --git a/executable_semantics/common/error.h b/executable_semantics/common/error.h index 15edc78230f9..678b8bcca4d1 100644 --- a/executable_semantics/common/error.h +++ b/executable_semantics/common/error.h @@ -33,9 +33,11 @@ class ErrorBuilder { return *this; } + // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns. operator Error() { return Error(message_); } template + // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns. operator ErrorOr() { return Error(message_); } @@ -81,21 +83,22 @@ class ErrorBuilder { #define MAKE_UNIQUE_NAME_IMPL(a, b, c) a##b##c #define MAKE_UNIQUE_NAME(a, b, c) MAKE_UNIQUE_NAME_IMPL(a, b, c) -#define RETURN_IF_ERROR_IMPL(unique_name, expr) \ - if (auto unique_name = (expr); !unique_name.ok()) { \ - return std::move(unique_name).error(); \ +#define RETURN_IF_ERROR_IMPL(unique_name, expr) \ + if (auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \ + !(unique_name).ok()) { \ + return std::move(unique_name).error(); \ } #define RETURN_IF_ERROR(expr) \ RETURN_IF_ERROR_IMPL( \ MAKE_UNIQUE_NAME(_llvm_error_line, __LINE__, __COUNTER__), expr) -#define ASSIGN_OR_RETURN_IMPL(unique_name, var, expr) \ - auto unique_name = (expr); \ - if (!unique_name.ok()) { \ - return std::move(unique_name).error(); \ - } \ - var = std::move(*unique_name); +#define ASSIGN_OR_RETURN_IMPL(unique_name, var, expr) \ + auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \ + if (!(unique_name).ok()) { \ + return std::move(unique_name).error(); \ + } \ + var = std::move(*(unique_name)); /* NOLINT(bugprone-macro-parentheses) */ #define ASSIGN_OR_RETURN(var, expr) \ ASSIGN_OR_RETURN_IMPL( \ diff --git a/executable_semantics/common/error_test.cpp b/executable_semantics/common/error_test.cpp index dcc3e131343b..4fddaa2c0cdc 100644 --- a/executable_semantics/common/error_test.cpp +++ b/executable_semantics/common/error_test.cpp @@ -9,8 +9,6 @@ namespace Carbon::Testing { namespace { -using ::testing::Eq; - auto MakeSuccess() -> ErrorOr { return Success(); } auto MakeError(std::string_view message) -> ErrorOr { diff --git a/executable_semantics/interpreter/action.cpp b/executable_semantics/interpreter/action.cpp index f3132224089f..a4f69dacc1a6 100644 --- a/executable_semantics/interpreter/action.cpp +++ b/executable_semantics/interpreter/action.cpp @@ -116,7 +116,7 @@ void Action::Print(llvm::raw_ostream& out) const { out << "ScopeAction"; } out << "<" << pos_ << ">"; - if (results_.size() > 0) { + if (!results_.empty()) { out << "("; llvm::ListSeparator sep; for (auto& result : results_) { diff --git a/executable_semantics/interpreter/type_checker.cpp b/executable_semantics/interpreter/type_checker.cpp index b1dddeec8dd2..8144d3fb03a1 100644 --- a/executable_semantics/interpreter/type_checker.cpp +++ b/executable_semantics/interpreter/type_checker.cpp @@ -1183,14 +1183,14 @@ auto TypeChecker::ExpectReturnOnAllPaths( // TODO: Add checking to function definitions to ensure that // all deduced type parameters will be deduced. auto TypeChecker::DeclareFunctionDeclaration(Nonnull f, - const ImplScope& impl_scope) + const ImplScope& enclosing_scope) -> ErrorOr { if (trace_) { llvm::outs() << "** declaring function " << f->name() << "\n"; } // Bring the deduced parameters into scope for (Nonnull deduced : f->deduced_parameters()) { - RETURN_IF_ERROR(TypeCheckExp(&deduced->type(), impl_scope)); + RETURN_IF_ERROR(TypeCheckExp(&deduced->type(), enclosing_scope)); SetConstantValue(deduced, arena_->New(deduced)); ASSIGN_OR_RETURN(Nonnull deduced_type, InterpExp(&deduced->type(), arena_, trace_)); @@ -1198,12 +1198,12 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull f, } // Type check the receiver pattern if (f->is_method()) { - RETURN_IF_ERROR(TypeCheckPattern(&f->me_pattern(), std::nullopt, impl_scope, - ValueCategory::Let)); + RETURN_IF_ERROR(TypeCheckPattern(&f->me_pattern(), std::nullopt, + enclosing_scope, ValueCategory::Let)); } // Type check the parameter pattern RETURN_IF_ERROR(TypeCheckPattern(&f->param_pattern(), std::nullopt, - impl_scope, ValueCategory::Let)); + enclosing_scope, ValueCategory::Let)); // Create the impl_bindings std::vector> impl_bindings; @@ -1221,7 +1221,7 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull f, return_expression.has_value()) { // We ignore the return value because return type expressions can't bring // new types into scope. - RETURN_IF_ERROR(TypeCheckExp(*return_expression, impl_scope)); + RETURN_IF_ERROR(TypeCheckExp(*return_expression, enclosing_scope)); // Should we be doing SetConstantValue instead? -Jeremy // And shouldn't the type of this be Type? ASSIGN_OR_RETURN(Nonnull ret_type, @@ -1237,13 +1237,13 @@ auto TypeChecker::DeclareFunctionDeclaration(Nonnull f, } // Bring the impl bindings into scope ImplScope function_scope; - function_scope.AddParent(&impl_scope); + function_scope.AddParent(&enclosing_scope); for (Nonnull impl_binding : impl_bindings) { function_scope.Add(impl_binding->interface(), *impl_binding->type_var()->constant_value(), impl_binding); } - RETURN_IF_ERROR(TypeCheckStmt(*f->body(), impl_scope)); + RETURN_IF_ERROR(TypeCheckStmt(*f->body(), enclosing_scope)); if (!f->return_term().is_omitted()) { RETURN_IF_ERROR(ExpectReturnOnAllPaths(f->body(), f->source_loc())); } @@ -1417,11 +1417,11 @@ auto TypeChecker::TypeCheckImplDeclaration(Nonnull impl_decl, } auto TypeChecker::DeclareChoiceDeclaration(Nonnull choice, - const ImplScope& impl_scope) + const ImplScope& enclosing_scope) -> ErrorOr { std::vector alternatives; for (Nonnull alternative : choice->alternatives()) { - RETURN_IF_ERROR(TypeCheckExp(&alternative->signature(), impl_scope)); + RETURN_IF_ERROR(TypeCheckExp(&alternative->signature(), enclosing_scope)); ASSIGN_OR_RETURN(auto signature, InterpExp(&alternative->signature(), arena_, trace_)); alternatives.push_back({.name = alternative->name(), .value = signature}); @@ -1432,8 +1432,8 @@ auto TypeChecker::DeclareChoiceDeclaration(Nonnull choice, return Success(); } -auto TypeChecker::TypeCheckChoiceDeclaration(Nonnull choice, - const ImplScope& impl_scope) +auto TypeChecker::TypeCheckChoiceDeclaration( + Nonnull /*choice*/, const ImplScope& /*impl_scope*/) -> ErrorOr { // Nothing to do here, but perhaps that will change in the future? return Success(); @@ -1504,34 +1504,35 @@ auto TypeChecker::TypeCheckDeclaration(Nonnull d, } auto TypeChecker::DeclareDeclaration(Nonnull d, - ImplScope& impl_scope) + ImplScope& enclosing_scope) -> ErrorOr { switch (d->kind()) { case DeclarationKind::InterfaceDeclaration: { auto& iface_decl = cast(*d); - RETURN_IF_ERROR(DeclareInterfaceDeclaration(&iface_decl, impl_scope)); + RETURN_IF_ERROR( + DeclareInterfaceDeclaration(&iface_decl, enclosing_scope)); break; } case DeclarationKind::ImplDeclaration: { auto& impl_decl = cast(*d); - RETURN_IF_ERROR(DeclareImplDeclaration(&impl_decl, impl_scope)); + RETURN_IF_ERROR(DeclareImplDeclaration(&impl_decl, enclosing_scope)); break; } case DeclarationKind::FunctionDeclaration: { auto& func_def = cast(*d); - RETURN_IF_ERROR(DeclareFunctionDeclaration(&func_def, impl_scope)); + RETURN_IF_ERROR(DeclareFunctionDeclaration(&func_def, enclosing_scope)); break; } case DeclarationKind::ClassDeclaration: { auto& class_decl = cast(*d); - RETURN_IF_ERROR(DeclareClassDeclaration(&class_decl, impl_scope)); + RETURN_IF_ERROR(DeclareClassDeclaration(&class_decl, enclosing_scope)); break; } case DeclarationKind::ChoiceDeclaration: { auto& choice = cast(*d); - RETURN_IF_ERROR(DeclareChoiceDeclaration(&choice, impl_scope)); + RETURN_IF_ERROR(DeclareChoiceDeclaration(&choice, enclosing_scope)); break; } @@ -1545,8 +1546,8 @@ auto TypeChecker::DeclareDeclaration(Nonnull d, } Expression& type = cast(var.binding().type()).expression(); - RETURN_IF_ERROR(TypeCheckPattern(&var.binding(), std::nullopt, impl_scope, - var.value_category())); + RETURN_IF_ERROR(TypeCheckPattern(&var.binding(), std::nullopt, + enclosing_scope, var.value_category())); ASSIGN_OR_RETURN(Nonnull declared_type, InterpExp(&type, arena_, trace_)); var.set_static_type(declared_type); diff --git a/executable_semantics/interpreter/type_checker.h b/executable_semantics/interpreter/type_checker.h index 4569e4365970..06933781557c 100644 --- a/executable_semantics/interpreter/type_checker.h +++ b/executable_semantics/interpreter/type_checker.h @@ -139,11 +139,11 @@ class TypeChecker { Nonnull>& dict, Nonnull type) -> Nonnull; - // Sets named_entity.constant_value() to `value`. Can be called multiple - // times on the same named_entity, so long as it is always called with + // Sets value_node.constant_value() to `value`. Can be called multiple + // times on the same value_node, so long as it is always called with // the same value. template - void SetConstantValue(Nonnull named_entity, Nonnull value); + void SetConstantValue(Nonnull value_node, Nonnull value); void PrintConstants(llvm::raw_ostream& out); diff --git a/executable_semantics/interpreter/value.cpp b/executable_semantics/interpreter/value.cpp index 82ef26e16522..09c6a60ff26b 100644 --- a/executable_semantics/interpreter/value.cpp +++ b/executable_semantics/interpreter/value.cpp @@ -266,7 +266,7 @@ void Value::Print(llvm::raw_ostream& out) const { case Value::Kind::FunctionType: { const auto& fn_type = cast(*this); out << "fn "; - if (fn_type.deduced().size() > 0) { + if (!fn_type.deduced().empty()) { out << "["; unsigned int i = 0; for (Nonnull deduced : fn_type.deduced()) { diff --git a/executable_semantics/interpreter/value.h b/executable_semantics/interpreter/value.h index 693af7e17511..0190fe33522d 100644 --- a/executable_semantics/interpreter/value.h +++ b/executable_semantics/interpreter/value.h @@ -525,8 +525,6 @@ class Witness : public Value { Nonnull declaration_; }; -auto FieldTypes(const NominalClassType&) -> std::vector; - // A choice type. class ChoiceType : public Value { public: diff --git a/toolchain/common/yaml_test_helpers.cpp b/toolchain/common/yaml_test_helpers.cpp index bfc0d639f7a6..6b68de78544d 100644 --- a/toolchain/common/yaml_test_helpers.cpp +++ b/toolchain/common/yaml_test_helpers.cpp @@ -72,9 +72,9 @@ auto operator<<(std::ostream& os, const Value& v) -> std::ostream& { // Variant visitor that prints the value in the form of code to recreate the // value. struct Printer { - auto operator()(NullValue) -> void { out << "Yaml::NullValue()"; } - auto operator()(AliasValue) -> void { out << "Yaml::AliasValue()"; } - auto operator()(ErrorValue) -> void { out << "Yaml::ErrorValue()"; } + auto operator()(NullValue /*v*/) -> void { out << "Yaml::NullValue()"; } + auto operator()(AliasValue /*v*/) -> void { out << "Yaml::AliasValue()"; } + auto operator()(ErrorValue /*v*/) -> void { out << "Yaml::ErrorValue()"; } auto operator()(const ScalarValue& v) -> void { out << std::quoted(v); } auto operator()(const MappingValue& v) -> void { out << "Yaml::MappingValue{"; diff --git a/toolchain/common/yaml_test_helpers.h b/toolchain/common/yaml_test_helpers.h index 82d162ac92e3..684d9bd442e4 100644 --- a/toolchain/common/yaml_test_helpers.h +++ b/toolchain/common/yaml_test_helpers.h @@ -60,10 +60,12 @@ namespace Carbon::Testing::Yaml { struct EmptyComparable { - friend auto operator==(EmptyComparable, EmptyComparable) -> bool { + friend auto operator==(EmptyComparable /*lhs*/, EmptyComparable /*rhs*/) + -> bool { return true; } - friend auto operator!=(EmptyComparable, EmptyComparable) -> bool { + friend auto operator!=(EmptyComparable /*lhs*/, EmptyComparable /*rhs*/) + -> bool { return false; } }; diff --git a/toolchain/diagnostics/null_diagnostics.h b/toolchain/diagnostics/null_diagnostics.h index 9fa3b55e44fc..9348a06e8139 100644 --- a/toolchain/diagnostics/null_diagnostics.h +++ b/toolchain/diagnostics/null_diagnostics.h @@ -13,7 +13,9 @@ template inline auto NullDiagnosticLocationTranslator() -> DiagnosticLocationTranslator& { struct Translator : DiagnosticLocationTranslator { - auto GetLocation(LocationT) -> Diagnostic::Location override { return {}; } + auto GetLocation(LocationT /*loc*/) -> Diagnostic::Location override { + return {}; + } }; static auto* translator = new Translator; return *translator; diff --git a/toolchain/lexer/numeric_literal.cpp b/toolchain/lexer/numeric_literal.cpp index c9b3d5400b03..d6ac89010e99 100644 --- a/toolchain/lexer/numeric_literal.cpp +++ b/toolchain/lexer/numeric_literal.cpp @@ -118,7 +118,8 @@ auto LexedNumericLiteral::Lex(llvm::StringRef source_text) // // TODO(zygoloid): Update lexical rules to specify that a numeric literal // cannot be immediately followed by an alphanumeric character. - int i = 1, n = source_text.size(); + int i = 1; + int n = source_text.size(); for (; i != n; ++i) { char c = source_text[i]; if (IsAlnum(c) || c == '_') { @@ -305,7 +306,7 @@ auto LexedNumericLiteral::Parser::GetExponent() -> llvm::APInt { // include a sign bit. Also make sure the exponent isn't too narrow so // the calculation below can't lose information through overflow. if (exponent.isSignBitSet() || exponent.getBitWidth() < 64) { - exponent = exponent.zext(std::max(64u, exponent.getBitWidth() + 1)); + exponent = exponent.zext(std::max(64U, exponent.getBitWidth() + 1)); } if (exponent_is_negative_) { exponent.negate(); diff --git a/toolchain/lexer/tokenized_buffer.cpp b/toolchain/lexer/tokenized_buffer.cpp index ddd5c69c5c14..ebff79ca599c 100644 --- a/toolchain/lexer/tokenized_buffer.cpp +++ b/toolchain/lexer/tokenized_buffer.cpp @@ -106,7 +106,7 @@ class TokenizedBuffer::Lexer { // Consumes (and discard) a valid token to construct a result // indicating a token has been produced. Relies on implicit conversions. // NOLINTNEXTLINE(google-explicit-constructor) - LexResult(Token) : LexResult(true) {} + LexResult(Token /*discarded_token*/) : LexResult(true) {} // Returns a result indicating no token was produced. static auto NoMatch() -> LexResult { return LexResult(false); } diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index a42b66222f32..25ff8060795d 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -382,7 +382,7 @@ class TokenizedBuffer { // Map the given position within the source buffer into a diagnostic // location. - auto GetLocation(const char* pos) -> Diagnostic::Location override; + auto GetLocation(const char* loc) -> Diagnostic::Location override; private: TokenizedBuffer* buffer_; @@ -396,7 +396,7 @@ class TokenizedBuffer { struct PrintWidths { // Widens `this` to the maximum of `this` and `new_width` for each // dimension. - auto Widen(const PrintWidths& new_width) -> void; + auto Widen(const PrintWidths& widths) -> void; int index; int kind; diff --git a/toolchain/lexer/tokenized_buffer_test.cpp b/toolchain/lexer/tokenized_buffer_test.cpp index 04704bb17dab..d2d8ac908455 100644 --- a/toolchain/lexer/tokenized_buffer_test.cpp +++ b/toolchain/lexer/tokenized_buffer_test.cpp @@ -919,7 +919,7 @@ TEST_F(LexerTest, TypeLiterals) { auto token_i20 = buffer.tokens().begin() + 2; EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i20), 20); auto token_i999999999999 = buffer.tokens().begin() + 3; - EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i999999999999), 999999999999ull); + EXPECT_EQ(buffer.GetTypeLiteralSize(*token_i999999999999), 999999999999ULL); auto token_u1 = buffer.tokens().begin() + 6; EXPECT_EQ(buffer.GetTypeLiteralSize(*token_u1), 1); auto token_u64 = buffer.tokens().begin() + 7; diff --git a/toolchain/parser/parse_test_helpers.h b/toolchain/parser/parse_test_helpers.h index 70af7bd291c4..64259fd3c9f0 100644 --- a/toolchain/parser/parse_test_helpers.h +++ b/toolchain/parser/parse_test_helpers.h @@ -289,8 +289,10 @@ auto MatchNode(Args... args) -> ExpectedNode { void UpdateExpectationsForArg(std::string text) { expected.text = std::move(text); } - void UpdateExpectationsForArg(HasErrorTag) { expected.has_error = true; } - void UpdateExpectationsForArg(AnyChildrenTag) { + void UpdateExpectationsForArg(HasErrorTag /*tag*/) { + expected.has_error = true; + } + void UpdateExpectationsForArg(AnyChildrenTag /*tag*/) { expected.skip_subtree = true; } void UpdateExpectationsForArg(ExpectedNode node) { diff --git a/toolchain/parser/parser_impl.h b/toolchain/parser/parser_impl.h index bb6d06b376ba..13cdb467aeb1 100644 --- a/toolchain/parser/parser_impl.h +++ b/toolchain/parser/parser_impl.h @@ -20,7 +20,7 @@ class ParseTree::Parser { // Parses the tokens into a parse tree, emitting any errors encountered. // // This is the entry point to the parser implementation. - static auto Parse(TokenizedBuffer& tokens, TokenDiagnosticEmitter& de) + static auto Parse(TokenizedBuffer& tokens, TokenDiagnosticEmitter& emitter) -> ParseTree; private: