Update files and clang-tidy config to pass with clang-tidy-20 (#4691)

Disables three new warnings because they lean more towards style
conflicts than fixes. I've brought these up on #style.

Other than that, mostly fixing basic issues, and things that
clang-tidy-20 seems to fire where clang-tiday-16 didn't. One particular
curious case is `llvm::StringLiteral::data()` uses, which are flagged as
not strictly null-terminated; I'm switching to `const char*` in those
spots which matches `llvm::formatv`'s format argument, but feels worse.

I'm removing `run_clang_tidy.py` here because I'm observing it give
fewer warnings than `bazel build --config=clang-tidy -k
//toolchain/...`. The latter matches how we enforce in GitHub actions
(and also caches results, and suppresses output for files that have no
issues), so I'm dropping the bespoke script.

.clang-tidy

@@ -29,12 +29,15 @@ Checks:
   # Disabled due to the implied style choices.
   - '-modernize-return-braced-init-list'
   - '-modernize-use-default-member-init'
+  - '-modernize-use-integer-sign-comparison'
   - '-modernize-use-emplace'
+  - '-readability-avoid-nested-conditional-operator'
   - '-readability-convert-member-functions-to-static'
   - '-readability-else-after-return'
   - '-readability-identifier-length'
   - '-readability-implicit-bool-conversion'
   - '-readability-make-member-function-const'
+  - '-readability-math-missing-parentheses'
   - '-readability-static-definition-in-anonymous-namespace'
   - '-readability-use-anyofallof'
 

common/hashing.h

@@ -591,6 +591,7 @@ inline auto MapToRawDataType(const T& value) -> const T& {
   // This overload should never be selected for `std::nullptr_t`, so
   // static_assert to get some better compiler error messages.
   static_assert(!std::same_as<T, std::nullptr_t>);
+  // NOLINTNEXTLINE(bugprone-return-const-ref-from-parameter)
   return value;
 }
 inline auto MapToRawDataType(std::nullptr_t /*value*/) -> const void* {

common/indirect_value_test.cpp

@@ -85,6 +85,7 @@ TEST(IndirectValueTest, MutableArrow) {
 
 TEST(IndirectValueTest, CopyConstruct) {
   IndirectValue<TestValue> v1;
+  // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
   auto v2 = v1;
   EXPECT_EQ(v1->state, "default constructed");
   EXPECT_EQ(v2->state, "copy constructed");

common/raw_hashtable_metadata_group_benchmark.cpp

@@ -189,6 +189,7 @@ const auto bench_metadata = BuildBenchMetadata<Kind>();
 // group.
 template <BenchKind Kind, typename GroupT = MetadataGroup>
 static void BM_LoadMatch(benchmark::State& s) {
+  // NOLINTNEXTLINE(google-readability-casting): Same as on `bench_metadata`.
   BenchMetadata bm = bench_metadata<Kind>[0];
 
   // We want to make the index used by the next iteration of the benchmark have

scripts/run_clang_tidy.py

@@ -1,61 +0,0 @@
-#!/usr/bin/env python3
-
-"""Runs clang-tidy over all Carbon files."""
-
-__copyright__ = """
-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
-"""
-
-import argparse
-import os
-import re
-import subprocess
-
-from pathlib import Path
-
-
-def main() -> None:
-    parser = argparse.ArgumentParser(__doc__)
-    # Copied from run-clang-tidy.py for forwarding.
-    parser.add_argument("-fix", action="store_true", help="Apply fix-its")
-    # Local flags.
-    parser.add_argument("files", nargs="*", help="Files to fix")
-    parsed_args = parser.parse_args()
-
-    # If files are passed in, resolve them; otherwise, add a path filter.
-    if parsed_args.files:
-        files = [str(Path(f).resolve()) for f in parsed_args.files]
-    else:
-        files = ["^(?!.*/(bazel-|third_party)).*$"]
-
-    # Set the repo root as the working directory.
-    os.chdir(Path(__file__).parents[1])
-    # Ensure create_compdb has been run.
-    subprocess.check_call(["./scripts/create_compdb.py"])
-
-    # Use the run-clang-tidy version that should be with the rest of the clang
-    # toolchain. This exposes us to version skew with user-installed clang
-    # versions, but avoids version skew between the script and clang-tidy
-    # itself.
-    with Path(
-        "./external/_main~clang_toolchain_extension~bazel_cc_toolchain/"
-        "clang_detected_variables.bzl"
-    ).open() as f:
-        clang_vars = f.read()
-    clang_bindir_match = re.search(r"clang_bindir = \"(.*)\"", clang_vars)
-    assert clang_bindir_match is not None, clang_vars
-
-    args = [str(Path(clang_bindir_match[1]).joinpath("run-clang-tidy"))]
-
-    # Forward flags.
-    if parsed_args.fix:
-        args.append("-fix")
-
-    # Run clang-tidy from clang-tools-extra.
-    exit(subprocess.call(args + files))
-
-
-if __name__ == "__main__":
-    main()

testing/base/source_gen.h

@@ -54,7 +54,7 @@ namespace Carbon::Testing {
 // identifiers that should be generalized.
 class SourceGen {
  public:
-  enum class Language {
+  enum class Language : uint8_t {
     Carbon,
     Cpp,
   };

testing/file_test/autoupdate.h

@@ -105,7 +105,9 @@ class FileTestAutoupdater {
   };
 
   // A CHECK line which is integrated into autoupdate output.
-  class CheckLine : public FileTestLineBase {
+  //
+  // `final` because we use pointer arithmetic on this type.
+  class CheckLine final : public FileTestLineBase {
    public:
     // RE2 is passed by a pointer because it doesn't support std::optional.
     explicit CheckLine(FileAndLineNumber file_and_line_number, std::string line)

testing/file_test/file_test_base.cpp

@@ -99,7 +99,7 @@ static auto CompareFailPrefix(llvm::StringRef filename, bool success) -> void {
 }
 
 // Modes for GetBazelCommand.
-enum class BazelMode {
+enum class BazelMode : uint8_t {
   Autoupdate,
   Dump,
   Test,
@@ -1081,9 +1081,9 @@ static auto Main(int argc, char** argv) -> int {
     llvm::errs() << "\nDone!\n";
     return EXIT_SUCCESS;
   } else {
-    for (llvm::StringRef test_name : tests) {
+    for (const std::string& test_name : tests) {
       testing::RegisterTest(
-          test_factory.name, test_name.data(), nullptr, test_name.data(),
+          test_factory.name, test_name.c_str(), nullptr, test_name.c_str(),
           __FILE__, __LINE__,
           [&test_factory, &exe_path, test_name = test_name]() {
             return test_factory.factory_fn(exe_path, nullptr, test_name);

testing/file_test/line.h

@@ -31,7 +31,9 @@ class FileTestLineBase : public Printable<FileTestLineBase> {
 };
 
 // A line in the original file test.
-class FileTestLine : public FileTestLineBase {
+//
+// `final` because we use pointer arithmetic on this type.
+class FileTestLine final : public FileTestLineBase {
  public:
   explicit FileTestLine(int file_number, int line_number, llvm::StringRef line)
       : FileTestLineBase(file_number, line_number), line_(line) {}

toolchain/check/param_and_arg_refs_stack.h

@@ -73,7 +73,7 @@ class ParamAndArgRefsStack {
 
   // Prints the stack for a stack dump.
   auto PrintForStackDump(int indent, llvm::raw_ostream& output) const -> void {
-    return stack_.PrintForStackDump(indent, output);
+    stack_.PrintForStackDump(indent, output);
   }
 
  private:

toolchain/check/pattern_match.cpp

@@ -35,7 +35,7 @@ static auto GetPrettyName(Context& context, ParamPattern param_pattern)
 namespace {
 
 // Selects between the different kinds of pattern matching.
-enum class MatchKind {
+enum class MatchKind : uint8_t {
   // Caller pattern matching occurs on the caller side of a function call, and
   // is responsible for matching the argument expression against the portion
   // of the pattern above the ParamPattern insts.

toolchain/driver/compile_benchmark.cpp

@@ -54,7 +54,7 @@ class CompileBenchmark {
 };
 
 // An enumerator used to select compilation phases to benchmark.
-enum class Phase {
+enum class Phase : uint8_t {
   Lex,
   Parse,
   Check,

toolchain/language_server/server.cpp

@@ -30,13 +30,13 @@ Server::Server(std::FILE* input_stream, llvm::raw_ostream& output_stream)
 
 auto Server::Run() -> ErrorOr<Success> {
   llvm::Error err = transport_->loop(*this);
-  if (err.success()) {
-    return Success();
-  } else {
+  if (err) {
     std::string str;
     llvm::raw_string_ostream out(str);
     out << err;
     return Error(str);
+  } else {
+    return Success();
   }
 }
 

toolchain/lex/lex.cpp

@@ -660,6 +660,7 @@ static auto DispatchNext(Lexer& lexer, llvm::StringRef source_text,
     // that because this is a must-tail return, this cannot fail to tail-call
     // and will not grow the stack. This is in essence a loop with dynamic
     // tail dispatch to the next stage of the loop.
+    // NOLINTNEXTLINE(readability-avoid-return-with-void-value): For musttail.
     [[clang::musttail]] return DispatchTable[static_cast<unsigned char>(
         source_text[position])](lexer, source_text, position);
   }

toolchain/lex/tokenized_buffer_benchmark.cpp

@@ -610,6 +610,7 @@ template <const DispatchTableT& Table>
 auto BasicDispatch(ssize_t& index, const char* text, char* buffer) -> void {
   *buffer = text[index];
   ++index;
+  // NOLINTNEXTLINE(readability-avoid-return-with-void-value): For musttail.
   [[clang::musttail]] return Table[static_cast<unsigned char>(text[index])](
       index, text, buffer);
 }
@@ -620,6 +621,7 @@ auto SpecializedDispatch(ssize_t& index, const char* text, char* buffer)
   CARBON_CHECK(C == text[index]);
   *buffer = C;
   ++index;
+  // NOLINTNEXTLINE(readability-avoid-return-with-void-value): For musttail.
   [[clang::musttail]] return Table[static_cast<unsigned char>(text[index])](
       index, text, buffer);
 }

toolchain/lex/tokenized_buffer_test.cpp

@@ -1215,14 +1215,16 @@ TEST_F(LexerTest, IndentedComments) {
     std::string simd_source =
         source +
         "\"Add a bunch of padding so that SIMD logic shouldn't hit EOF\"";
-    auto& simd_buffer = compile_helper_.GetTokenizedBuffer(source);
+    auto& simd_buffer = compile_helper_.GetTokenizedBuffer(simd_source);
     ASSERT_FALSE(simd_buffer.has_errors());
     EXPECT_THAT(simd_buffer.comments_size(), Eq(1));
   }
 }
 
 TEST_F(LexerTest, MultipleComments) {
-  constexpr llvm::StringLiteral Format = R"(
+  // TODO: Switch format to `llvm::StringLiteral` if
+  // `llvm::StringLiteral::c_str` is added.
+  constexpr char Format[] = R"(
 {0}
   {1}
 
@@ -1252,7 +1254,7 @@ x
       "//\n"
       "// Valid\n",
       "// This uses a high indent, which stops SIMD.\n", "//\n"};
-  std::string source = llvm::formatv(Format.data(), Comments[0], Comments[1],
+  std::string source = llvm::formatv(Format, Comments[0], Comments[1],
                                      Comments[2], Comments[3], Comments[4])
                            .str();
 

toolchain/parse/extract.cpp

@@ -76,11 +76,13 @@ class NodeExtractor {
                             std::tuple<U...>* /*type*/) -> std::optional<T>;
 
   // Split out trace logic. The noinline saves a few seconds on compilation.
+  // TODO: Switch format to `llvm::StringLiteral` if
+  // `llvm::StringLiteral::c_str` is added.
   template <typename... ArgT>
-  [[clang::noinline]] auto MaybeTrace(llvm::StringLiteral format,
-                                      ArgT... args) const -> void {
+  [[clang::noinline]] auto MaybeTrace(const char* format, ArgT... args) const
+      -> void {
     if (trace_) {
-      *trace_ << llvm::formatv(format.data(), args...);
+      *trace_ << llvm::formatv(format, args...);
     }
   }
 

toolchain/parse/handle_import_and_package.cpp

@@ -15,8 +15,8 @@ namespace Carbon::Parse {
 // Provides common error exiting logic that skips to the semi, if present.
 static auto OnParseError(Context& context, Context::StateStackEntry state,
                          NodeKind declaration) -> void {
-  return context.AddNode(declaration, context.SkipPastLikelyEnd(state.token),
-                         /*has_error=*/true);
+  context.AddNode(declaration, context.SkipPastLikelyEnd(state.token),
+                  /*has_error=*/true);
 }
 
 // Determines whether the specified modifier appears within the introducer of

toolchain/sem_ir/function.h

@@ -15,7 +15,7 @@ namespace Carbon::SemIR {
 // Function-specific fields.
 struct FunctionFields {
   // Kinds of virtual modifiers that can apply to functions.
-  enum class VirtualModifier { None, Virtual, Abstract, Impl };
+  enum class VirtualModifier : uint8_t { None, Virtual, Abstract, Impl };
 
   // The following members always have values, and do not change throughout the
   // lifetime of the function.

toolchain/sem_ir/name_scope.h

@@ -107,7 +107,7 @@ class NameScope : public Printable<NameScope> {
   auto AddImportIRScope(
       const std::pair<SemIR::ImportIRId, SemIR::NameScopeId>& import_ir_scope)
       -> void {
-    return import_ir_scopes_.push_back(import_ir_scope);
+    import_ir_scopes_.push_back(import_ir_scope);
   }
 
  private:

toolchain/sem_ir/singleton_insts.h

@@ -28,16 +28,16 @@ static constexpr std::array SingletonInstKinds = {
 };
 
 // Returns true if the InstKind is a singleton.
-inline constexpr auto IsSingletonInstKind(InstKind kind) -> bool;
+constexpr auto IsSingletonInstKind(InstKind kind) -> bool;
 
 // Provides the InstId for singleton instructions. These are exposed as
 // `InstT::SingletonInstId` in `typed_insts.h`.
 template <InstKind::RawEnumType Kind>
   requires(IsSingletonInstKind(InstKind::Make(Kind)))
-inline constexpr auto MakeSingletonInstId() -> InstId;
+constexpr auto MakeSingletonInstId() -> InstId;
 
 // Returns true if the InstId corresponds to a singleton inst.
-inline constexpr auto IsSingletonInstId(InstId id) -> bool {
+constexpr auto IsSingletonInstId(InstId id) -> bool {
   return id.index >= 0 &&
          id.index < static_cast<int32_t>(SingletonInstKinds.size());
 }
@@ -47,7 +47,7 @@ inline constexpr auto IsSingletonInstId(InstId id) -> bool {
 namespace Internal {
 
 // Returns the index for a singleton instruction, or -1 if it's not a singleton.
-inline constexpr auto GetSingletonInstIndex(InstKind kind) -> int32_t {
+constexpr auto GetSingletonInstIndex(InstKind kind) -> int32_t {
   for (int32_t i = 0; i < static_cast<int32_t>(SingletonInstKinds.size());
        ++i) {
     if (SingletonInstKinds[i] == kind) {
@@ -59,13 +59,13 @@ inline constexpr auto GetSingletonInstIndex(InstKind kind) -> int32_t {
 
 }  // namespace Internal
 
-inline constexpr auto IsSingletonInstKind(InstKind kind) -> bool {
+constexpr auto IsSingletonInstKind(InstKind kind) -> bool {
   return Internal::GetSingletonInstIndex(kind) >= 0;
 }
 
 template <InstKind::RawEnumType Kind>
   requires(IsSingletonInstKind(InstKind::Make(Kind)))
-inline constexpr auto MakeSingletonInstId() -> InstId {
+constexpr auto MakeSingletonInstId() -> InstId {
   auto index = Internal::GetSingletonInstIndex(InstKind::Make(Kind));
   return InstId(index);
 }

toolchain/testing/coverage_helper.h

@@ -21,15 +21,17 @@ namespace Carbon::Testing {
 //
 // should_be_covered should return false when a kind is either untestable or not
 // yet tested.
+//
+// TODO: Switch `kind_pattern` to `llvm::StringLiteral` if
+// `llvm::StringLiteral::c_str` is added.
 template <typename KindT>
 auto TestKindCoverage(const std::string& manifest_path,
-                      llvm::StringLiteral kind_pattern,
-                      llvm::ArrayRef<KindT> kinds,
+                      const char* kind_pattern, llvm::ArrayRef<KindT> kinds,
                       llvm::ArrayRef<KindT> untested_kinds) {
   std::ifstream manifest_in(manifest_path.c_str());
   ASSERT_TRUE(manifest_in.good());
 
-  RE2 kind_re(kind_pattern.data());
+  RE2 kind_re(kind_pattern);
   ASSERT_TRUE(kind_re.ok()) << kind_re.error();
 
   Set<std::string> covered_kinds;

toolchain/testing/file_test.cpp

@@ -130,7 +130,7 @@ class ToolchainFileTest : public FileTestBase {
           R"((FileEnd.*column: |FileStart.*line: )( *\d+))");
       RE2::Replace(&check_line, file_token_re, R"(\1{{ *\\d+}})");
     } else {
-      return FileTestBase::DoExtraCheckReplacements(check_line);
+      FileTestBase::DoExtraCheckReplacements(check_line);
     }
   }