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

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, just a couple of other comments inline.



================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:230
+
+  std::shared_ptr<CompilerInvocation> getInvocationPtr() {
     assert(Invocation && "Compiler instance has no invocation!");
----------------
Is `get*Ptr()` already used in CompilerInstance? If so, matching that style sounds great. Otherwise I have a slight preference for `getShared*()`.


================
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;
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > 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?
> It can't be `const &` for two reasons:
> 
> 1. The current code value-initializes `ModuleDeps` in two places and truly initializes it later in `ModuleDepCollectorPP::handleTopLevelModule`. We'd have to go with something like `optional<reference_wrapper<CompilerInvocation>>` to be able to delay the initialization.
> 2. The lifetime of the `CompilerInvocation` is a bit wild, but the reference most likely wouldn't outlive `ModuleDeps` (depends on the client).
> 
> I think the best solution would be to extract the `shared_ptr` from `CompilerInstance`, share it across `ModuleDeps` instances and only do a deep copy when actually generating the command line.
Using shared_ptr seems right given the lifetime issues.

FWIW, I think `const CompilerInvocation*` is probably simpler to work with than `Optional<reference_wrapper<const CompilerInvocation>>`.


================
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);
 
----------------
jansvoboda11 wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > 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.
> > That's intentional. The deep copy is performed inside the function.
> > 
> > Shouldn't the move constructor of `CompilerInvocation` be defaulted?
> s/defaulted/implicitly-defined/
Explicitly declaring a copy constructor suppresses the implicit move constructor. But adding a move with `= default` is probably all that's needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100534