[PATCH] D100534: [clang][deps] Generate the full command-line for modules
Jan Svoboda via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 16 05:08:19 PDT 2021
jansvoboda11 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;
----------------
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.
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:25
+
+ // Remove options incompatible with explicit module build.
+ CI.getFrontendOpts().Inputs.clear();
----------------
dexonsmith wrote:
> Should this call any of the `resetNonModularOptions()` functions, or are those intentionally omitted?
My intent for this patch was to simply take the work @Bigcheese has done downstream with `-remove-preceeding-explicit-module-build-incompatible-options` and port it to `CompilerInvocation`.
Calling `resetNonModularOptions()` here would be the next step I intend to do in a future patch.
I also think disabling implicit module maps here is not correct, as that seems to disable inferred top-level modules.
================
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();
+
----------------
dexonsmith wrote:
> Should `FrontendOpts` gain a `resetNonModularOptions()`?
Most likely yes. I'd like to look into it in a next patch.
================
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:
> 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100534/new/
https://reviews.llvm.org/D100534
More information about the cfe-commits
mailing list