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

@@ -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:

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);
 }
 

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<const char*>& emitter) const -> Value;
 
  private:
-  LexedNumericLiteral() {}
+  LexedNumericLiteral() = default;
 
   class Parser;
 

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<LexedNumericLiteral::Value> {
   return VariantWith<LexedNumericLiteral::RealValue>(AllOf(
       Field(&LexedNumericLiteral::RealValue::radix, real_matcher.radix),

toolchain/lexer/string_literal.cpp

@@ -79,7 +79,7 @@ struct MismatchedIndentInString : SimpleDiagnostic<MismatchedIndentInString> {
 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;
   }

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.

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_

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.

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.
   //

toolchain/parser/parse_test_helpers.h

@@ -7,6 +7,7 @@
 
 #include <ostream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #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.

toolchain/parser/parser_impl.cpp

@@ -650,7 +650,7 @@ auto ParseTree::Parser::ParseParenExpression() -> llvm::Optional<Node> {
       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<Node> {
@@ -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<Node> {
@@ -985,7 +985,7 @@ auto ParseTree::Parser::ParseIfStatement() -> llvm::Optional<Node> {
     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<Node> {
@@ -994,7 +994,7 @@ auto ParseTree::Parser::ParseWhileStatement() -> llvm::Optional<Node> {
   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<Node> {

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<TokenKind> kinds) const -> bool {
+  [[nodiscard]] auto NextTokenIsOneOf(
+      std::initializer_list<TokenKind> kinds) const -> bool {
     return NextTokenKind().IsOneOf(kinds);
   }
 

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 <higher precedence>, <lower precedence>
     // 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

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<Associativity>(GetPriority(*this, *this));
   }
 

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());

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);
   }