Repository: carbon-language/carbon-lang
File: testing/file_test/file_test_base.cpp (C++)

Code before:
  bool single_threaded = SingleThreaded(tests_);

  std::unique_ptr<llvm::ThreadPoolInterface> pool;
  if (single_threaded) {
    pool = std::make_unique<llvm::SingleThreadExecutor>();
  } else {
    // Enable the CRC for use in `RunSingleTest`.
    llvm::CrashRecoveryContext::Enable();
    pool = std::make_unique<llvm::DefaultThreadPool>(llvm::ThreadPoolStrategy{
        .ThreadsRequested = absl::GetFlag(FLAGS_threads)});
  }
  if (!absl::GetFlag(FLAGS_dump_output)) {
    llvm::errs() << "Running tests with " << pool->getMaxConcurrency()
                 << " thread(s)\n";
  }

  // Guard access to both `llvm::errs` and `crashed`.
  bool crashed = false;
  std::mutex output_mutex;

  for (auto& test : tests_) {
    if (!test.registered_test->should_run()) {
      continue;
    }

    pool->async([&single_threaded, &output_mutex, &crashed, &test] {
      // If any thread crashed, don't try running more.
      if (std::unique_lock<std::mutex> lock(output_mutex); crashed) {
        return;
      }

      if (!RunSingleTest(test, single_threaded, output_mutex)) {
        std::unique_lock<std::mutex> lock(output_mutex);
        crashed = true;
      }
    });
  }

  pool->wait();
  if (crashed) {
    // Abort rather than returning so that we don't get a LeakSanitizer report.
    // We expect to have leaked memory if one or more of our tests crashed.
    std::abort();
  }
  llvm::errs() << "\nDone!\n";
}

// Implements main() within the Carbon::Testing namespace for convenience.
static auto Main(int argc, char** argv) -> ErrorOr<int> {
  Carbon::InitLLVM init_llvm(argc, argv);
  testing::InitGoogleTest(&argc, argv);

Review comment (bug):
```suggestion
    pool->async([single_threaded, &output_mutex, &crashed, &test] {
```

I don't think this `&` is necessary, and it makes me worry that `single_threaded` might get mutated.

FWIW I think it would be fine to use `[&]` here instead (it's clear that the lambda's scope is limited enough to make that safe); what threw me off was the fact that the code seems to go out of its way to specify a reference capture for this specific variable.

Code after:
auto FileTestEventListener::OnTestProgramStart(
    const testing::UnitTest& /*unit_test*/) -> void {
  bool single_threaded = SingleThreaded(tests_);

  std::unique_ptr<llvm::ThreadPoolInterface> pool;
  if (single_threaded) {
    pool = std::make_unique<llvm::SingleThreadExecutor>();
  } else {
    // Enable the CRC for use in `RunSingleTest`.
    llvm::CrashRecoveryContext::Enable();
    pool = std::make_unique<llvm::DefaultThreadPool>(llvm::ThreadPoolStrategy{
        .ThreadsRequested = absl::GetFlag(FLAGS_threads)});
  }
  if (!absl::GetFlag(FLAGS_dump_output)) {
    llvm::errs() << "Running tests with " << pool->getMaxConcurrency()
                 << " thread(s)\n";
  }

  // Guard access to output (stdout and stderr).
  std::mutex output_mutex;
  std::atomic<bool> crashed = false;

  for (auto& test : tests_) {
    if (!test.registered_test->should_run()) {
      continue;
    }

    pool->async([&] {
      // If any thread crashed, don't try running more.
      if (crashed) {
        return;
      }

      if (!RunSingleTest(test, single_threaded, output_mutex)) {
        crashed = true;
      }
    });
  }

  pool->wait();
  if (crashed) {
    // Abort rather than returning so that we don't get a LeakSanitizer report.
    // We expect to have leaked memory if one or more of our tests crashed.
    std::abort();
  }
  llvm::errs() << "\nDone!\n";
}

// Implements main() within the Carbon::Testing namespace for convenience.
static auto Main(int argc, char** argv) -> ErrorOr<int> {
  Carbon::InitLLVM init_llvm(argc, argv);