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