[clang][ScanDeps] Use an options struct for the scanning service

[Bigcheese]

Upcoming patches will be adding more options to this constructor which will end up requiring lots of test changes to handle new default arguments. This uses a struct instead so that only the settings relevant to the test can be set. This also avoids the problem of the meaning of a bool argument changing.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Michael Spencer (Bigcheese)

Changes

Upcoming patches will be adding more options to this constructor which will end up requiring lots of test changes to handle new default arguments. This uses a struct instead so that only the settings relevant to the test can be set. This also avoids the problem of the meaning of a bool argument changing.

---

Full diff: https://github.com/llvm/llvm-project/pull/167964.diff

4 Files Affected:

• (modified) clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h (+28-22)
• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp (+2-6)
• (modified) clang/tools/clang-scan-deps/ClangScanDeps.cpp (+5-2)
• (modified) clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp (+6-6)

diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
index 4e97c7bc9f36e..699bbfc691c47 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
@@ -78,26 +78,38 @@ enum class ScanningOptimizations {
 
 #undef DSS_LAST_BITMASK_ENUM
 
+struct DependencyScanningServiceOptions {
+  DependencyScanningServiceOptions(ScanningMode Mode,
+                                   ScanningOutputFormat Format)
+      : Mode(Mode), Format(Format) {}
+
+  ScanningMode Mode;
+  ScanningOutputFormat Format;
+  /// How to optimize the modules' command-line arguments.
+  ScanningOptimizations OptimizeArgs = ScanningOptimizations::Default;
+  /// Whether to set up command-lines to load PCM files eagerly.
+  bool EagerLoadModules = false;
+  /// Whether to trace VFS accesses.
+  bool TraceVFS = false;
+  std::time_t BuildSessionTimestamp =
+      llvm::sys::toTimeT(std::chrono::system_clock::now());
+};
+
 /// The dependency scanning service contains shared configuration and state that
 /// is used by the individual dependency scanning workers.
 class DependencyScanningService {
 public:
-  DependencyScanningService(
-      ScanningMode Mode, ScanningOutputFormat Format,
-      ScanningOptimizations OptimizeArgs = ScanningOptimizations::Default,
-      bool EagerLoadModules = false, bool TraceVFS = false,
-      std::time_t BuildSessionTimestamp =
-          llvm::sys::toTimeT(std::chrono::system_clock::now()));
+  DependencyScanningService(const DependencyScanningServiceOptions &Options);
 
-  ScanningMode getMode() const { return Mode; }
+  ScanningMode getMode() const { return Options.Mode; }
 
-  ScanningOutputFormat getFormat() const { return Format; }
+  ScanningOutputFormat getFormat() const { return Options.Format; }
 
-  ScanningOptimizations getOptimizeArgs() const { return OptimizeArgs; }
+  ScanningOptimizations getOptimizeArgs() const { return Options.OptimizeArgs; }
 
-  bool shouldEagerLoadModules() const { return EagerLoadModules; }
+  bool shouldEagerLoadModules() const { return Options.EagerLoadModules; }
 
-  bool shouldTraceVFS() const { return TraceVFS; }
+  bool shouldTraceVFS() const { return Options.TraceVFS; }
 
   DependencyScanningFilesystemSharedCache &getSharedCache() {
     return SharedCache;
@@ -105,23 +117,17 @@ class DependencyScanningService {
 
   ModuleCacheEntries &getModuleCacheEntries() { return ModCacheEntries; }
 
-  std::time_t getBuildSessionTimestamp() const { return BuildSessionTimestamp; }
+  std::time_t getBuildSessionTimestamp() const {
+    return Options.BuildSessionTimestamp;
+  }
 
 private:
-  const ScanningMode Mode;
-  const ScanningOutputFormat Format;
-  /// Whether to optimize the modules' command-line arguments.
-  const ScanningOptimizations OptimizeArgs;
-  /// Whether to set up command-lines to load PCM files eagerly.
-  const bool EagerLoadModules;
-  /// Whether to trace VFS accesses.
-  const bool TraceVFS;
+  const DependencyScanningServiceOptions Options;
+
   /// The global file system cache.
   DependencyScanningFilesystemSharedCache SharedCache;
   /// The global module cache entries.
   ModuleCacheEntries ModCacheEntries;
-  /// The build session timestamp.
-  std::time_t BuildSessionTimestamp;
 };
 
 } // end namespace dependencies
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
index 7f40c99f07287..f6671250fe33a 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
@@ -13,9 +13,5 @@ using namespace tooling;
 using namespace dependencies;
 
 DependencyScanningService::DependencyScanningService(
-    ScanningMode Mode, ScanningOutputFormat Format,
-    ScanningOptimizations OptimizeArgs, bool EagerLoadModules, bool TraceVFS,
-    std::time_t BuildSessionTimestamp)
-    : Mode(Mode), Format(Format), OptimizeArgs(OptimizeArgs),
-      EagerLoadModules(EagerLoadModules), TraceVFS(TraceVFS),
-      BuildSessionTimestamp(BuildSessionTimestamp) {}
+    const DependencyScanningServiceOptions &Options)
+    : Options(Options) {}
diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
index 5f5bf42df5e6b..d7d6693e85084 100644
--- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -1171,8 +1171,11 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
     });
   };
 
-  DependencyScanningService Service(ScanMode, Format, OptimizeArgs,
-                                    EagerLoadModules, /*TraceVFS=*/Verbose);
+  DependencyScanningServiceOptions Options{ScanMode, Format};
+  Options.OptimizeArgs = OptimizeArgs;
+  Options.EagerLoadModules = EagerLoadModules;
+  Options.TraceVFS = Verbose;
+  DependencyScanningService Service(Options);
 
   llvm::Timer T;
   T.startTimer();
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
index 4523af33e3c28..455aaceb0cc4e 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
@@ -231,8 +231,8 @@ TEST(DependencyScanner, ScanDepsWithFS) {
   VFS->addFile(TestPath, 0,
                llvm::MemoryBuffer::getMemBuffer("#include \"header.h\"\n"));
 
-  DependencyScanningService Service(ScanningMode::DependencyDirectivesScan,
-                                    ScanningOutputFormat::Make);
+  DependencyScanningService Service(
+      {ScanningMode::DependencyDirectivesScan, ScanningOutputFormat::Make});
   DependencyScanningTool ScanTool(Service, VFS);
 
   std::string DepFile;
@@ -289,8 +289,8 @@ TEST(DependencyScanner, ScanDepsWithModuleLookup) {
 
   auto InterceptFS = llvm::makeIntrusiveRefCnt<InterceptorFS>(VFS);
 
-  DependencyScanningService Service(ScanningMode::DependencyDirectivesScan,
-                                    ScanningOutputFormat::Make);
+  DependencyScanningService Service(
+      {ScanningMode::DependencyDirectivesScan, ScanningOutputFormat::Make});
   DependencyScanningTool ScanTool(Service, InterceptFS);
 
   // This will fail with "fatal error: module 'Foo' not found" but it doesn't
@@ -321,8 +321,8 @@ TEST(DependencyScanner, ScanDepsWithDiagConsumer) {
                llvm::MemoryBuffer::getMemBuffer("#include \"header.h\"\n"));
   VFS->addFile(AsmPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
 
-  DependencyScanningService Service(ScanningMode::DependencyDirectivesScan,
-                                    ScanningOutputFormat::Make);
+  DependencyScanningService Service(
+      {ScanningMode::DependencyDirectivesScan, ScanningOutputFormat::Make});
   DependencyScanningWorker Worker(Service, VFS);
 
   llvm::DenseSet<ModuleID> AlreadySeen;

[jansvoboda11]

Looks good, thanks!

[jansvoboda11]

Is this a separate type from the service so that service still only exposes const getters and cannot be mutated?

[jansvoboda11]

I wonder if we could move this into the .cpp to lose the dependency on the chrono header. Probably doesn't matter much.