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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
