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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks for the review!



================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:230
+
+  std::shared_ptr<CompilerInvocation> getInvocationPtr() {
     assert(Invocation && "Compiler instance has no invocation!");
----------------
dexonsmith wrote:
> Is `get*Ptr()` already used in CompilerInstance? If so, matching that style sounds great. Otherwise I have a slight preference for `getShared*()`.
Yes, this naming pattern is already used:
* `std::shared_ptr<HeaderSearchOptions> getHeaderSearchOptsPtr() const`
* `std::shared_ptr<Preprocessor> getPreprocessorPtr()`
and `getShared*()` is not being used.


================
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);
 
----------------
dexonsmith wrote:
> 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.
I see, thanks for pointing that out. I explicitly defaulted move constructor/assignment in D100473.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100534