From a857b7ea1adc660c82f5c41b001771cd947df641 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Mon, 14 Jun 2021 19:46:49 -0700 Subject: [PATCH] Cleanup or suppress numerous `clang-tidy` issues. (#577) This gets us to a nearly clean state across the toolchain. A couple of these are checks that I don't think we want to try to rigidly use and I've disabled them completely. Others I've added relevant `NOLINT` style suppressions or applied the automatic fix suggested by `clang-tidy`. The implicit conversions that are allowed here with `NOLINT` are probably worth at least a tiny bit of scrutiny to see if we could replace the construct with something more direct without undue effort and no longer need the implicit conversion. But until then, it seemed fine to suppress. --- .clang-tidy | 11 ++++++----- toolchain/lexer/character_set.h | 18 ++++++++++-------- toolchain/lexer/numeric_literal.h | 4 ++-- toolchain/lexer/numeric_literal_test.cpp | 2 +- toolchain/lexer/string_literal.cpp | 4 ++-- toolchain/lexer/string_literal.h | 4 ++-- toolchain/lexer/test_helpers.h | 11 +++++------ toolchain/lexer/tokenized_buffer.cpp | 5 +++-- toolchain/lexer/tokenized_buffer.h | 8 ++++---- toolchain/parser/parse_test_helpers.h | 6 ++++-- toolchain/parser/parser_impl.cpp | 10 +++++----- toolchain/parser/parser_impl.h | 9 ++++++--- toolchain/parser/precedence.cpp | 6 +++--- toolchain/parser/precedence.h | 5 ++++- toolchain/parser/precedence_test.cpp | 1 - toolchain/source/source_buffer.cpp | 3 +++ 16 files changed, 60 insertions(+), 47 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index b078ecc2d414..2ee99f1ee966 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -4,11 +4,12 @@ --- Checks: - -*, bugprone-*, google-*, -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-*, - -modernize-avoid-c-arrays, performance-*, + -*, bugprone-*, -bugprone-narrowing-conversions, google-*, + -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-*, -modernize-avoid-c-arrays, + -modernize-use-default-member-init, performance-*, readability-braces-around-statements, readability-identifier-naming WarningsAsErrors: true CheckOptions: diff --git a/toolchain/lexer/character_set.h b/toolchain/lexer/character_set.h index 304d5e2766c3..101eb3029197 100644 --- a/toolchain/lexer/character_set.h +++ b/toolchain/lexer/character_set.h @@ -18,12 +18,12 @@ namespace Carbon { // // Alphabetical characters are permitted at the start of identifiers. This // currently includes 'A'..'Z' and 'a'..'z'. -inline bool IsAlpha(char c) { return llvm::isAlpha(c); } +inline auto IsAlpha(char c) -> bool { return llvm::isAlpha(c); } // Is this a decimal digit according to Carbon's lexical rules? // // This currently includes '0'..'9'. -inline bool IsDecimalDigit(char c) { return llvm::isDigit(c); } +inline auto IsDecimalDigit(char c) -> bool { return llvm::isDigit(c); } // Is this an alphanumeric character according to Carbon's lexical rules? // @@ -33,7 +33,7 @@ inline bool IsDecimalDigit(char c) { return llvm::isDigit(c); } // // Note that '_' is not considered alphanumeric, despite in most circumstances // being a valid continuation character of an identifier or numeric literal. -inline bool IsAlnum(char c) { return llvm::isAlnum(c); } +inline auto IsAlnum(char c) -> bool { return llvm::isAlnum(c); } // Is this a hexadecimal digit according to Carbon's lexical rules? // @@ -42,7 +42,7 @@ inline bool IsAlnum(char c) { return llvm::isAlnum(c); } // // Note that lowercase 'a'..'f' are currently not considered hexadecimal digits // in any context. -inline bool IsUpperHexDigit(char c) { +inline auto IsUpperHexDigit(char c) -> bool { return ('0' <= c && c <= '9') || ('A' <= c && c <= 'F'); } @@ -50,23 +50,25 @@ inline bool IsUpperHexDigit(char c) { // // Lowercase letters in numeric literals can be followed by `+` or `-` to // extend the literal. -inline bool IsLower(char c) { return 'a' <= c && c <= 'z'; } +inline auto IsLower(char c) -> bool { return 'a' <= c && c <= 'z'; } // Is this character considered to be horizontal whitespace? // // Such characters can appear in the indentation of a line. -inline bool IsHorizontalWhitespace(char c) { return c == ' ' || c == '\t'; } +inline auto IsHorizontalWhitespace(char c) -> bool { + return c == ' ' || c == '\t'; +} // Is this character considered to be vertical whitespace? // // Such characters are considered to terminate lines. -inline bool IsVerticalWhitespace(char c) { return c == '\n'; } +inline auto IsVerticalWhitespace(char c) -> bool { return c == '\n'; } // Is this character considered to be whitespace? // // Changes here will need matching changes in // `TokenizedBuffer::Lexer::SkipWhitespace`. -inline bool IsSpace(char c) { +inline auto IsSpace(char c) -> bool { return IsHorizontalWhitespace(c) || IsVerticalWhitespace(c); } diff --git a/toolchain/lexer/numeric_literal.h b/toolchain/lexer/numeric_literal.h index b33d5833f0e0..93443127dad5 100644 --- a/toolchain/lexer/numeric_literal.h +++ b/toolchain/lexer/numeric_literal.h @@ -19,7 +19,7 @@ namespace Carbon { class LexedNumericLiteral { public: // Get the text corresponding to this literal. - auto Text() const -> llvm::StringRef { return text; } + [[nodiscard]] auto Text() const -> llvm::StringRef { return text; } // Extract a numeric literal from the given text, if it has a suitable form. // @@ -52,7 +52,7 @@ class LexedNumericLiteral { auto ComputeValue(DiagnosticEmitter& emitter) const -> Value; private: - LexedNumericLiteral() {} + LexedNumericLiteral() = default; class Parser; diff --git a/toolchain/lexer/numeric_literal_test.cpp b/toolchain/lexer/numeric_literal_test.cpp index 758efa990bed..ee0fb48fe1e7 100644 --- a/toolchain/lexer/numeric_literal_test.cpp +++ b/toolchain/lexer/numeric_literal_test.cpp @@ -75,7 +75,7 @@ struct RealMatcher { }; // Matcher for a real literal value. -auto HasRealValue(RealMatcher real_matcher) +auto HasRealValue(const RealMatcher& real_matcher) -> Matcher { return VariantWith(AllOf( Field(&LexedNumericLiteral::RealValue::radix, real_matcher.radix), diff --git a/toolchain/lexer/string_literal.cpp b/toolchain/lexer/string_literal.cpp index b29b6d099c54..1d57ea844dfe 100644 --- a/toolchain/lexer/string_literal.cpp +++ b/toolchain/lexer/string_literal.cpp @@ -79,7 +79,7 @@ struct MismatchedIndentInString : SimpleDiagnostic { static auto TakeMultiLineStringLiteralPrefix(llvm::StringRef source_text) -> llvm::StringRef { llvm::StringRef remaining = source_text; - if (!remaining.consume_front("\"\"\"")) { + if (!remaining.consume_front(R"(""")")) { return llvm::StringRef(); } @@ -113,7 +113,7 @@ auto LexedStringLiteral::Lex(llvm::StringRef source_text) bool multi_line = !multi_line_prefix.empty(); if (multi_line) { source_text = source_text.drop_front(multi_line_prefix.size()); - terminator = "\"\"\""; + terminator = R"(""")"; } else if (!source_text.consume_front("\"")) { return llvm::None; } diff --git a/toolchain/lexer/string_literal.h b/toolchain/lexer/string_literal.h index 79715bcd73c9..8766f8918ea8 100644 --- a/toolchain/lexer/string_literal.h +++ b/toolchain/lexer/string_literal.h @@ -13,10 +13,10 @@ namespace Carbon { class LexedStringLiteral { public: // Get the text corresponding to this literal. - auto Text() const -> llvm::StringRef { return text; } + [[nodiscard]] auto Text() const -> llvm::StringRef { return text; } // Determine whether this is a multi-line string literal. - auto IsMultiLine() const -> bool { return multi_line; } + [[nodiscard]] auto IsMultiLine() const -> bool { return multi_line; } // Extract a string literal token from the given text, if it has a suitable // form. diff --git a/toolchain/lexer/test_helpers.h b/toolchain/lexer/test_helpers.h index 75ef4f709b78..623c6cdcbe7b 100644 --- a/toolchain/lexer/test_helpers.h +++ b/toolchain/lexer/test_helpers.h @@ -13,8 +13,7 @@ #include "llvm/Support/FormatVariadic.h" #include "toolchain/diagnostics/diagnostic_emitter.h" -namespace Carbon { -namespace Testing { +namespace Carbon::Testing { // A diagnostic translator for tests that lex a single token. Produces // locations such as "`12.5`:1:3" to refer to the third character in the token. @@ -23,7 +22,8 @@ class SingleTokenDiagnosticTranslator public: // Form a translator for a given token. The string provided here must refer // to the same character array that we are going to lex. - SingleTokenDiagnosticTranslator(llvm::StringRef token) : token(token) {} + explicit SingleTokenDiagnosticTranslator(llvm::StringRef token) + : token(token) {} auto GetLocation(const char* pos) -> Diagnostic::Location override { assert(llvm::is_sorted(std::array{token.begin(), pos, token.end()}) && @@ -47,14 +47,13 @@ class SingleTokenDiagnosticTranslator } private: - auto SynthesizeFilename() const -> std::string { + [[nodiscard]] auto SynthesizeFilename() const -> std::string { return llvm::formatv("`{0}`", token); } llvm::StringRef token; }; -} // namespace Testing -} // namespace Carbon +} // namespace Carbon::Testing #endif // TOOLCHAIN_LEXER_TOKENIZED_BUFFER_TEST_HELPERS_H_ diff --git a/toolchain/lexer/tokenized_buffer.cpp b/toolchain/lexer/tokenized_buffer.cpp index 64f2fdb2e0cb..282569d42760 100644 --- a/toolchain/lexer/tokenized_buffer.cpp +++ b/toolchain/lexer/tokenized_buffer.cpp @@ -123,11 +123,12 @@ class TokenizedBuffer::Lexer { public: // Consumes (and discard) a valid token to construct a result - // indicating a token has been produced. + // indicating a token has been produced. Relies on implicit conversions. + // NOLINTNEXTLINE(google-explicit-constructor) LexResult(Token) : LexResult(true) {} // Returns a result indicating no token was produced. - static LexResult NoMatch() { return LexResult(false); } + static auto NoMatch() -> LexResult { return LexResult(false); } // Tests whether a token was produced by the lexing routine, and // the lexer can continue forming tokens. diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index d5001eb5ba51..feda2e9d357d 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -215,16 +215,16 @@ class TokenizedBuffer { public: // The mantissa, represented as an unsigned integer. - auto Mantissa() const -> const llvm::APInt& { + [[nodiscard]] auto Mantissa() const -> const llvm::APInt& { return buffer->literal_int_storage[literal_index]; } // The exponent, represented as a signed integer. - auto Exponent() const -> const llvm::APInt& { + [[nodiscard]] auto Exponent() const -> const llvm::APInt& { return buffer->literal_int_storage[literal_index + 1]; } // If false, the value is mantissa * 2^exponent. // If true, the value is mantissa * 10^exponent. - auto IsDecimal() const -> bool { return is_decimal; } + [[nodiscard]] auto IsDecimal() const -> bool { return is_decimal; } private: friend class TokenizedBuffer; @@ -291,7 +291,7 @@ class TokenizedBuffer { [[nodiscard]] auto GetRealLiteral(Token token) const -> RealLiteralValue; // Returns the value of a `StringLiteral()` token. - auto GetStringLiteral(Token token) const -> llvm::StringRef; + [[nodiscard]] auto GetStringLiteral(Token token) const -> llvm::StringRef; // Returns the closing token matched with the given opening token. // diff --git a/toolchain/parser/parse_test_helpers.h b/toolchain/parser/parse_test_helpers.h index 5dd46b1ab4b7..c7e3dfe86eca 100644 --- a/toolchain/parser/parse_test_helpers.h +++ b/toolchain/parser/parse_test_helpers.h @@ -7,6 +7,7 @@ #include #include +#include #include #include "gmock/gmock.h" @@ -318,8 +319,9 @@ auto MatchNode(Args... args) -> ExpectedNode { #include "parse_node_kind.def" // Helper for matching a designator `lhs.rhs`. -auto MatchDesignator(ExpectedNode lhs, std::string rhs) -> ExpectedNode { - return MatchDesignatorExpression(std::move(lhs), MatchDesignatedName(rhs)); +inline auto MatchDesignator(ExpectedNode lhs, std::string rhs) -> ExpectedNode { + return MatchDesignatorExpression(std::move(lhs), + MatchDesignatedName(std::move(rhs))); } // Helper for matching a function parameter list. diff --git a/toolchain/parser/parser_impl.cpp b/toolchain/parser/parser_impl.cpp index 5e6564cc8c0d..d26e8256e996 100644 --- a/toolchain/parser/parser_impl.cpp +++ b/toolchain/parser/parser_impl.cpp @@ -650,7 +650,7 @@ auto ParseTree::Parser::ParseParenExpression() -> llvm::Optional { ParseCloseParen(open_paren, ParseNodeKind::ParenExpressionEnd()); return AddNode(ParseNodeKind::ParenExpression(), open_paren, start, - /*has_errors=*/!expr || !close_paren); + /*has_error=*/!expr || !close_paren); } auto ParseTree::Parser::ParsePrimaryExpression() -> llvm::Optional { @@ -971,7 +971,7 @@ auto ParseTree::Parser::ParseParenCondition(TokenKind introducer) ParseCloseParen(*open_paren, ParseNodeKind::ConditionEnd()); return AddNode(ParseNodeKind::Condition(), *open_paren, start, - /*has_errors=*/!expr || !close_paren); + /*has_error=*/!expr || !close_paren); } auto ParseTree::Parser::ParseIfStatement() -> llvm::Optional { @@ -985,7 +985,7 @@ auto ParseTree::Parser::ParseIfStatement() -> llvm::Optional { else_has_errors = !ParseStatement(); } return AddNode(ParseNodeKind::IfStatement(), if_token, start, - /*has_errors=*/!cond || !then_case || else_has_errors); + /*has_error=*/!cond || !then_case || else_has_errors); } auto ParseTree::Parser::ParseWhileStatement() -> llvm::Optional { @@ -994,7 +994,7 @@ auto ParseTree::Parser::ParseWhileStatement() -> llvm::Optional { auto cond = ParseParenCondition(TokenKind::WhileKeyword()); auto body = ParseStatement(); return AddNode(ParseNodeKind::WhileStatement(), while_token, start, - /*has_errors=*/!cond || !body); + /*has_error=*/!cond || !body); } auto ParseTree::Parser::ParseKeywordStatement(ParseNodeKind kind, @@ -1020,7 +1020,7 @@ auto ParseTree::Parser::ParseKeywordStatement(ParseNodeKind kind, {.preceding = keyword_kind}); // FIXME: Try to skip to a semicolon to recover. } - return AddNode(kind, keyword, start, /*has_errors=*/!semi || arg_error); + return AddNode(kind, keyword, start, /*has_error=*/!semi || arg_error); } auto ParseTree::Parser::ParseStatement() -> llvm::Optional { diff --git a/toolchain/parser/parser_impl.h b/toolchain/parser/parser_impl.h index cb3a676cfe87..7861fc82db09 100644 --- a/toolchain/parser/parser_impl.h +++ b/toolchain/parser/parser_impl.h @@ -34,16 +34,19 @@ class ParseTree::Parser { } // Gets the kind of the next token to be consumed. - auto NextTokenKind() const -> TokenKind { return tokens.GetKind(*position); } + [[nodiscard]] auto NextTokenKind() const -> TokenKind { + return tokens.GetKind(*position); + } // Tests whether the next token to be consumed is of the specified kind. - auto NextTokenIs(TokenKind kind) const -> bool { + [[nodiscard]] auto NextTokenIs(TokenKind kind) const -> bool { return NextTokenKind() == kind; } // Tests whether the next token to be consumed is of any of the specified // kinds. - auto NextTokenIsOneOf(std::initializer_list kinds) const -> bool { + [[nodiscard]] auto NextTokenIsOneOf( + std::initializer_list kinds) const -> bool { return NextTokenKind().IsOneOf(kinds); } diff --git a/toolchain/parser/precedence.cpp b/toolchain/parser/precedence.cpp index 5c6c5ebd181a..8ad701215cc0 100644 --- a/toolchain/parser/precedence.cpp +++ b/toolchain/parser/precedence.cpp @@ -44,7 +44,7 @@ constexpr int8_t NumPrecedenceLevels = Lowest + 1; // A precomputed lookup table determining the relative precedence of two // precedence groups. struct OperatorPriorityTable { - constexpr OperatorPriorityTable() : table{} { + constexpr OperatorPriorityTable() : table() { // Start with a list of , // relationships. MarkHigherThan({Highest}, {NumericPrefix, BitwisePrefix, LogicalPrefix, @@ -310,8 +310,8 @@ auto PrecedenceGroup::ForTrailing(TokenKind kind, bool infix) auto PrecedenceGroup::GetPriority(PrecedenceGroup left, PrecedenceGroup right) -> OperatorPriority { - static constexpr OperatorPriorityTable lookup; - return lookup.table[left.level][right.level]; + static constexpr OperatorPriorityTable Lookup; + return Lookup.table[left.level][right.level]; } } // namespace Carbon diff --git a/toolchain/parser/precedence.h b/toolchain/parser/precedence.h index 7f995b9bda50..883dcc0c480a 100644 --- a/toolchain/parser/precedence.h +++ b/toolchain/parser/precedence.h @@ -30,6 +30,9 @@ enum class Associativity : int8_t { // A precedence group associated with an operator or expression. class PrecedenceGroup { private: + // We rely on implicit conversions via `int8_t` for enumerators defined in the + // implementation. + // NOLINTNEXTLINE(google-explicit-constructor) PrecedenceGroup(int8_t level) : level(level) {} public: @@ -75,7 +78,7 @@ class PrecedenceGroup { -> OperatorPriority; // Get the associativity of this precedence group. - Associativity GetAssociativity() const { + [[nodiscard]] auto GetAssociativity() const -> Associativity { return static_cast(GetPriority(*this, *this)); } diff --git a/toolchain/parser/precedence_test.cpp b/toolchain/parser/precedence_test.cpp index ad1a172ba7f1..252bf2794da6 100644 --- a/toolchain/parser/precedence_test.cpp +++ b/toolchain/parser/precedence_test.cpp @@ -12,7 +12,6 @@ namespace Carbon { namespace { using ::testing::Eq; -using ::testing::Ne; TEST(PrecedenceTest, OperatorsAreRecognized) { EXPECT_TRUE(PrecedenceGroup::ForLeading(TokenKind::Minus()).hasValue()); diff --git a/toolchain/source/source_buffer.cpp b/toolchain/source/source_buffer.cpp index b6c2f3d2df43..0c269a2a9a06 100644 --- a/toolchain/source/source_buffer.cpp +++ b/toolchain/source/source_buffer.cpp @@ -61,6 +61,9 @@ auto SourceBuffer::CreateFromFile(llvm::StringRef filename) MAP_PRIVATE | MAP_POPULATE, #endif file_descriptor, /*offset=*/0); + // The `MAP_FAILED` macro may expand to a cast to pointer that `clang-tidy` + // complains about. + // NOLINTNEXTLINE(performance-no-int-to-ptr) if (mapped_text == MAP_FAILED) { return ErrnoToError(errno); }