Addressing code review comments.

qiongsiwu · qiongsiwu · commit e076b56b772f · 2025-09-30T16:36:33.000-07:00
diff --git a/clang/lib/Tooling/DependencyScanning/CMakeLists.txt b/clang/lib/Tooling/DependencyScanning/CMakeLists.txt
@@ -24,6 +24,5 @@ add_clang_library(clangDependencyScanning
   clangFrontend
   clangLex
   clangSerialization
-  clangTooling
   ${LLVM_PTHREAD_LIB}
   )
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp
@@ -334,22 +334,9 @@ class ScanningDependencyDirectivesGetter : public DependencyDirectivesGetter {
     return DepFS->getDirectiveTokens(File.getName());
   }
 };
-} // namespace
-
-std::unique_ptr<DiagnosticOptions>
-clang::tooling::dependencies::createDiagOptions(
-    const std::vector<std::string> &CommandLine) {
-  std::vector<const char *> CLI;
-  for (const std::string &Arg : CommandLine)
-    CLI.push_back(Arg.c_str());
-  auto DiagOpts = CreateAndPopulateDiagOpts(CLI);
-  sanitizeDiagOpts(*DiagOpts);
-  return DiagOpts;
-}
 
 /// Sanitize diagnostic options for dependency scan.
-void clang::tooling::dependencies::sanitizeDiagOpts(
-    DiagnosticOptions &DiagOpts) {
+void sanitizeDiagOpts(DiagnosticOptions &DiagOpts) {
   // Don't print 'X warnings and Y errors generated'.
   DiagOpts.ShowCarets = false;
   // Don't write out diagnostic file.
@@ -368,6 +355,31 @@ void clang::tooling::dependencies::sanitizeDiagOpts(
         .Default(true);
   });
 }
+} // namespace
+
+std::unique_ptr<DiagnosticOptions>
+clang::tooling::dependencies::createDiagOptions(
+    const std::vector<std::string> &CommandLine) {
+  std::vector<const char *> CLI;
+  for (const std::string &Arg : CommandLine)
+    CLI.push_back(Arg.c_str());
+  auto DiagOpts = CreateAndPopulateDiagOpts(CLI);
+  sanitizeDiagOpts(*DiagOpts);
+  return DiagOpts;
+}
+
+clang::tooling::dependencies::DignosticsEngineWithCCommandLineAndDiagOpts::
+    DignosticsEngineWithCCommandLineAndDiagOpts(
+        const std::vector<std::string> CommandLine,
+        IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS, DiagnosticConsumer &DC)
+    : CCommandLine(CommandLine.size(), nullptr) {
+  llvm::transform(CommandLine, CCommandLine.begin(),
+                  [](const std::string &Str) { return Str.c_str(); });
+  DiagOpts = CreateAndPopulateDiagOpts(CCommandLine);
+  sanitizeDiagOpts(*DiagOpts);
+  DiagEngine = CompilerInstance::createDiagnostics(*FS, *DiagOpts, &DC,
+                                                   /*ShouldOwnClient=*/false);
+}
 
 std::pair<std::unique_ptr<driver::Driver>, std::unique_ptr<driver::Compilation>>
 clang::tooling::dependencies::buildCompilation(
@@ -395,7 +407,7 @@ clang::tooling::dependencies::buildCompilation(
   }
 
   std::unique_ptr<driver::Compilation> Compilation(
-      Driver->BuildCompilation(llvm::ArrayRef(Argv)));
+      Driver->BuildCompilation(Argv));
   if (!Compilation)
     return std::make_pair(nullptr, nullptr);
 
@@ -405,45 +417,37 @@ clang::tooling::dependencies::buildCompilation(
   return std::make_pair(std::move(Driver), std::move(Compilation));
 }
 
-llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-clang::tooling::dependencies::initVFSForTUBufferScanning(
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-    std::optional<std::vector<std::string>> &ModifiedCommandLine,
+std::pair<IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::vector<std::string>>
+clang::tooling::dependencies::initVFSForTUBuferScanning(
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
     const std::vector<std::string> &CommandLine, StringRef WorkingDirectory,
-    std::optional<llvm::MemoryBufferRef> TUBuffer) {
+    llvm::MemoryBufferRef TUBuffer) {
   // Reset what might have been modified in the previous worker invocation.
   BaseFS->setCurrentWorkingDirectory(WorkingDirectory);
 
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> ModifiedFS;
-  if (TUBuffer) {
-    auto OverlayFS =
-        llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(BaseFS);
-    auto InMemoryFS =
-        llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-    InMemoryFS->setCurrentWorkingDirectory(WorkingDirectory);
-    auto InputPath = TUBuffer->getBufferIdentifier();
-    InMemoryFS->addFile(
-        InputPath, 0,
-        llvm::MemoryBuffer::getMemBufferCopy(TUBuffer->getBuffer()));
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> InMemoryOverlay =
-        InMemoryFS;
-
-    OverlayFS->pushOverlay(InMemoryOverlay);
-    ModifiedFS = OverlayFS;
-    ModifiedCommandLine = CommandLine;
-    ModifiedCommandLine->emplace_back(InputPath);
-
-    return ModifiedFS;
-  }
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> ModifiedFS;
+  auto OverlayFS =
+      llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(BaseFS);
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  InMemoryFS->setCurrentWorkingDirectory(WorkingDirectory);
+  auto InputPath = TUBuffer.getBufferIdentifier();
+  InMemoryFS->addFile(
+      InputPath, 0, llvm::MemoryBuffer::getMemBufferCopy(TUBuffer.getBuffer()));
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> InMemoryOverlay = InMemoryFS;
+
+  OverlayFS->pushOverlay(InMemoryOverlay);
+  ModifiedFS = OverlayFS;
+  auto ModifiedCommandLine = CommandLine;
+  ModifiedCommandLine.emplace_back(InputPath);
 
-  return BaseFS;
+  return std::make_pair(ModifiedFS, ModifiedCommandLine);
 }
 
-llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+std::pair<IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::vector<std::string>>
 clang::tooling::dependencies::initVFSForByNameScanning(
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-    std::vector<std::string> &CommandLine, StringRef WorkingDirectory,
-    StringRef FakeFileNamePrefix) {
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+    const std::vector<std::string> &CommandLine, StringRef WorkingDirectory,
+    StringRef ModuleName) {
   // Reset what might have been modified in the previous worker invocation.
   BaseFS->setCurrentWorkingDirectory(WorkingDirectory);
 
@@ -456,16 +460,16 @@ clang::tooling::dependencies::initVFSForByNameScanning(
   InMemoryFS->setCurrentWorkingDirectory(WorkingDirectory);
   SmallString<128> FakeInputPath;
   // TODO: We should retry the creation if the path already exists.
-  llvm::sys::fs::createUniquePath(FakeFileNamePrefix + "-%%%%%%%%.input",
-                                  FakeInputPath,
+  llvm::sys::fs::createUniquePath(ModuleName + "-%%%%%%%%.input", FakeInputPath,
                                   /*MakeAbsolute=*/false);
   InMemoryFS->addFile(FakeInputPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> InMemoryOverlay = InMemoryFS;
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> InMemoryOverlay = InMemoryFS;
   OverlayFS->pushOverlay(InMemoryOverlay);
 
-  CommandLine.emplace_back(FakeInputPath);
+  auto ModifiedCommandLine = CommandLine;
+  ModifiedCommandLine.emplace_back(FakeInputPath);
 
-  return OverlayFS;
+  return std::make_pair(OverlayFS, ModifiedCommandLine);
 }
 
 std::unique_ptr<CompilerInvocation>
@@ -487,7 +491,7 @@ bool clang::tooling::dependencies::initializeScanCompilerInstance(
     CompilerInstance &ScanInstance,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
     DiagnosticConsumer *DiagConsumer, DependencyScanningService &Service,
-    llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS) {
+    IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS) {
   ScanInstance.setBuildingModule(false);
 
   ScanInstance.createVirtualFileSystem(FS, DiagConsumer);
@@ -554,7 +558,7 @@ bool clang::tooling::dependencies::initializeScanCompilerInstance(
 }
 
 llvm::SmallVector<StringRef> clang::tooling::dependencies::computeStableDirs(
-    CompilerInstance &ScanInstance) {
+    const CompilerInstance &ScanInstance) {
   // Create a collection of stable directories derived from the ScanInstance
   // for determining whether module dependencies would fully resolve from
   // those directories.
@@ -600,7 +604,7 @@ void clang::tooling::dependencies::initializeModuleDepCollector(
   // and thus won't write out the extra '.d' files to disk.
   auto Opts = std::make_unique<DependencyOutputOptions>();
 
-  // We rely on the behaviour that that ScanInstance's Invocation instance's
+  // We rely on the behaviour that the ScanInstance's Invocation instance's
   // dependency output opts are cleared here.
   // TODO: we will need to preserve and recover the original dependency output
   // opts if we want to reuse ScanInstance.
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.h b/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.h
@@ -74,15 +74,28 @@ class DependencyScanningAction {
   bool DiagConsumerFinished = false;
 };
 
-// Helper functions
+// Helper functions and data types.
 std::unique_ptr<DiagnosticOptions>
 createDiagOptions(const std::vector<std::string> &CommandLine);
-void sanitizeDiagOpts(DiagnosticOptions &DiagOpts);
+
+struct DignosticsEngineWithCCommandLineAndDiagOpts {
+  // We need to bound the lifetime of the CCommandLine and the DiagOpts
+  // used to create the DiganosticsEngine with the DiagnosticsEngine itself.
+  std::vector<const char *> CCommandLine;
+  std::unique_ptr<DiagnosticOptions> DiagOpts;
+  IntrusiveRefCntPtr<DiagnosticsEngine> DiagEngine;
+
+  DignosticsEngineWithCCommandLineAndDiagOpts(
+      const std::vector<std::string> CommandLine,
+      IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS, DiagnosticConsumer &DC);
+};
 
 struct TextDiagnosticsPrinterWithOutput {
+  // We need to bound the lifetime of the data that supports the DiagPrinter
+  // with it together so they have the same lifetime.
   std::string DiagnosticOutput;
   llvm::raw_string_ostream DiagnosticsOS;
-  std::unique_ptr<clang::DiagnosticOptions> DiagOpts;
+  std::unique_ptr<DiagnosticOptions> DiagOpts;
   TextDiagnosticPrinter DiagPrinter;
 
   TextDiagnosticsPrinterWithOutput(const std::vector<std::string> &CommandLine)
@@ -99,25 +112,25 @@ std::unique_ptr<CompilerInvocation>
 createCompilerInvocation(const std::vector<std::string> &CommandLine,
                          DiagnosticsEngine &Diags);
 
-llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> initVFSForTUBufferScanning(
-    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-    std::optional<std::vector<std::string>> &ModifiedCommandLine,
-    const std::vector<std::string> &CommandLine, StringRef WorkingDirectory,
-    std::optional<llvm::MemoryBufferRef> TUBuffer);
+std::pair<IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::vector<std::string>>
+initVFSForTUBuferScanning(IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
+                          const std::vector<std::string> &CommandLine,
+                          StringRef WorkingDirectory,
+                          llvm::MemoryBufferRef TUBuffer);
 
-llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+std::pair<IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::vector<std::string>>
 initVFSForByNameScanning(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-                         std::vector<std::string> &CommandLine,
-                         StringRef WorkingDirectory,
-                         StringRef FakeFileNamePrefix);
+                         const std::vector<std::string> &CommandLine,
+                         StringRef WorkingDirectory, StringRef ModuleName);
 
 bool initializeScanCompilerInstance(
     CompilerInstance &ScanInstance,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
     DiagnosticConsumer *DiagConsumer, DependencyScanningService &Service,
     llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS);
 
-llvm::SmallVector<StringRef> computeStableDirs(CompilerInstance &ScanInstance);
+llvm::SmallVector<StringRef>
+computeStableDirs(const CompilerInstance &ScanInstance);
 
 std::optional<PrebuiltModulesAttrsMap>
 computePrebuiltModulesASTMap(CompilerInstance &ScanInstance,
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -8,29 +8,9 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "DependencyScannerImpl.h"
-#include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Basic/DiagnosticFrontend.h"
-#include "clang/Basic/DiagnosticSerialization.h"
-#include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
-#include "clang/Driver/Job.h"
 #include "clang/Driver/Tool.h"
-#include "clang/Frontend/CompilerInstance.h"
-#include "clang/Frontend/CompilerInvocation.h"
-#include "clang/Frontend/FrontendActions.h"
-#include "clang/Frontend/TextDiagnosticPrinter.h"
-#include "clang/Frontend/Utils.h"
-#include "clang/Lex/PreprocessorOptions.h"
-#include "clang/Serialization/ObjectFilePCHContainerReader.h"
-#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
-#include "clang/Tooling/DependencyScanning/InProcessModuleCache.h"
-#include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
-#include "llvm/ADT/IntrusiveRefCntPtr.h"
-#include "llvm/Support/Allocator.h"
-#include "llvm/Support/Error.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/TargetParser/Host.h"
-#include <optional>
 
 using namespace clang;
 using namespace tooling;
@@ -97,7 +77,7 @@ static bool forEachDriverJob(
     ArrayRef<std::string> ArgStrs, DiagnosticsEngine &Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
     llvm::function_ref<bool(const driver::Command &Cmd)> Callback) {
-  // Compilation owns a reference to the Driver, hence we need to
+  // Compilation holds a non-owning a reference to the Driver, hence we need to
   // keep the Driver alive when we use Compilation.
   auto [Driver, Compilation] = buildCompilation(ArgStrs, Diags, FS);
   if (!Compilation)
@@ -133,24 +113,20 @@ bool DependencyScanningWorker::scanDependencies(
     DependencyConsumer &Consumer, DependencyActionController &Controller,
     DiagnosticConsumer &DC, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
     std::optional<StringRef> ModuleName) {
-  std::vector<const char *> CCommandLine(CommandLine.size(), nullptr);
-  llvm::transform(CommandLine, CCommandLine.begin(),
-                  [](const std::string &Str) { return Str.c_str(); });
-  auto DiagOpts = CreateAndPopulateDiagOpts(CCommandLine);
-  sanitizeDiagOpts(*DiagOpts);
-  auto Diags = CompilerInstance::createDiagnostics(*FS, *DiagOpts, &DC,
-                                                   /*ShouldOwnClient=*/false);
-
+  DignosticsEngineWithCCommandLineAndDiagOpts DiagEngineWithCmdAndOpts(
+      CommandLine, FS, DC);
   DependencyScanningAction Action(Service, WorkingDirectory, Consumer,
                                   Controller, DepFS, ModuleName);
 
   bool Success = false;
   if (CommandLine[1] == "-cc1") {
-    Success = createAndRunToolInvocation(CommandLine, Action, FS,
-                                         PCHContainerOps, *Diags, Consumer);
+    Success = createAndRunToolInvocation(
+        CommandLine, Action, FS, PCHContainerOps,
+        *DiagEngineWithCmdAndOpts.DiagEngine, Consumer);
   } else {
     Success = forEachDriverJob(
-        CommandLine, *Diags, FS, [&](const driver::Command &Cmd) {
+        CommandLine, *DiagEngineWithCmdAndOpts.DiagEngine, FS,
+        [&](const driver::Command &Cmd) {
           if (StringRef(Cmd.getCreator().getName()) != "clang") {
             // Non-clang command. Just pass through to the dependency
             // consumer.
@@ -169,13 +145,15 @@ bool DependencyScanningWorker::scanDependencies(
           // system to ensure that any file system requests that
           // are made by the driver do not go through the
           // dependency scanning filesystem.
-          return createAndRunToolInvocation(std::move(Argv), Action, FS,
-                                            PCHContainerOps, *Diags, Consumer);
+          return createAndRunToolInvocation(
+              std::move(Argv), Action, FS, PCHContainerOps,
+              *DiagEngineWithCmdAndOpts.DiagEngine, Consumer);
         });
   }
 
   if (Success && !Action.hasScanned())
-    Diags->Report(diag::err_fe_expected_compiler_job)
+    DiagEngineWithCmdAndOpts.DiagEngine->Report(
+        diag::err_fe_expected_compiler_job)
         << llvm::join(CommandLine, " ");
 
   // Ensure finish() is called even if we never reached ExecuteAction().
@@ -189,23 +167,25 @@ bool DependencyScanningWorker::computeDependencies(
     StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
     DependencyConsumer &Consumer, DependencyActionController &Controller,
     DiagnosticConsumer &DC, std::optional<llvm::MemoryBufferRef> TUBuffer) {
-  std::optional<std::vector<std::string>> ModifiedCommandLine;
-  auto FinalFS = initVFSForTUBufferScanning(
-      BaseFS, ModifiedCommandLine, CommandLine, WorkingDirectory, TUBuffer);
-  const std::vector<std::string> &FinalCommandLine =
-      ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
-
-  return scanDependencies(WorkingDirectory, FinalCommandLine, Consumer,
-                          Controller, DC, FinalFS, /*ModuleName=*/std::nullopt);
+  if (TUBuffer) {
+    auto [FinalFS, FinalCommandLine] = initVFSForTUBuferScanning(
+        BaseFS, CommandLine, WorkingDirectory, *TUBuffer);
+    return scanDependencies(WorkingDirectory, FinalCommandLine, Consumer,
+                            Controller, DC, FinalFS,
+                            /*ModuleName=*/std::nullopt);
+  } else {
+    BaseFS->setCurrentWorkingDirectory(WorkingDirectory);
+    return scanDependencies(WorkingDirectory, CommandLine, Consumer, Controller,
+                            DC, BaseFS, /*ModuleName=*/std::nullopt);
+  }
 }
 
 bool DependencyScanningWorker::computeDependencies(
     StringRef WorkingDirectory, const std::vector<std::string> &CommandLine,
     DependencyConsumer &Consumer, DependencyActionController &Controller,
     DiagnosticConsumer &DC, StringRef ModuleName) {
-  auto ModifiedCommandLine = CommandLine;
-  auto OverlayFS = initVFSForByNameScanning(BaseFS, ModifiedCommandLine,
-                                            WorkingDirectory, ModuleName);
+  auto [OverlayFS, ModifiedCommandLine] = initVFSForByNameScanning(
+      BaseFS, CommandLine, WorkingDirectory, ModuleName);
 
   return scanDependencies(WorkingDirectory, ModifiedCommandLine, Consumer,
                           Controller, DC, OverlayFS, ModuleName);

Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,5 @@ add_clang_library(clangDependencyScanning`
`24`	`24`	`clangFrontend`
`25`	`25`	`clangLex`
`26`	`26`	`clangSerialization`
`27`		`- clangTooling`
`28`	`27`	`${LLVM_PTHREAD_LIB}`
`29`	`28`	`)`