[PATCH] D100534: [clang][deps] Generate the full command-line for modules

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:78-80
+  /// The compiler invocation associated with the translation unit that imports
+  /// this module.
+  CompilerInvocation Invocation;
----------------
Looks like this will be a deep copy, but it doesn't look like it's being modified. Can this just be a `const &`, taken in the `ModuleDeps` constructor? Or is there a lifetime reason this needs to be as it is?


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:25
+
+  // Remove options incompatible with explicit module build.
+  CI.getFrontendOpts().Inputs.clear();
----------------
Should this call any of the `resetNonModularOptions()` functions, or are those intentionally omitted?


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:26-27
+  // Remove options incompatible with explicit module build.
+  CI.getFrontendOpts().Inputs.clear();
+  CI.getFrontendOpts().OutputFile.clear();
+
----------------
Should `FrontendOpts` gain a `resetNonModularOptions()`?


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:62
     std::function<const ModuleDeps &(ModuleID)> LookupModuleDeps) const {
-  // TODO: Build full command line. That also means capturing the original
-  //       command line into NonPathCommandLine.
-
-  std::vector<std::string> Ret{
-      "-fno-implicit-modules",
-      "-fno-implicit-module-maps",
-  };
+  CompilerInvocation CI = getFullCommandLineCompilerInvocation(*this);
 
----------------
I think guaranteed copy elision means this won't be a deep copy of the return, but it might be nice to add a move constructor for `CompilerInvocation` so it's more obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100534/new/

https://reviews.llvm.org/D100534