jansvoboda11 added inline comments.
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:112
void dependencies::detail::collectPCMAndModuleMapPaths(
llvm::ArrayRef<ModuleID> Modules,
----------------
dexonsmith wrote:
> Should this be renamed?
Yes, that would make sense.
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:125-126
PCMPaths.push_back(LookupPCMPath(MID).str());
- if (!M.ClangModuleMapFile.empty())
- ModMapPaths.push_back(M.ClangModuleMapFile);
}
----------------
dexonsmith wrote:
> Can you clarify why this is safe to remove, even though sometimes we do need
> to add module map files?
This collects the module map files of transitive dependencies. We used it to
generate `-fmodule-map-file=` arguments for builds of modules.
However, with explicit modules, it's not necessary to provide
`-fmodule-map-file=` arguments of (transitive) dependencies at all. The module
map semantics are basically serialized in the PCM files themselves. That's why
this is safe to remove.
The new code below handles a special case (`[no_undeclared_includes]`) where we
still need to generate some `-fmodule-map-file=` arguments. Semantics of such
module maps will not be found in any PCM, since we don't import the modules
they describe.
Note that caller of this function used to pass
`FrontendOptions::ModuleMapFiles` member as `ModMapPaths`. The member is now
being initialized in the new code below.
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:269-273
+ // However, some module maps loaded implicitly during the dependency scan can
+ // describe anti-dependencies. That happens when the current module is marked
+ // as '[no_undeclared_includes]', doesn't 'use' module from such module map,
+ // but tries to import it anyway. We need to tell the explicit build about
+ // such module map for it to have the same semantics as the implicit build.
----------------
dexonsmith wrote:
> Is there another long-term solution to this that could be pointed at with a
> FIXME? E.g., could the module map be encoded redundantly here? If so, what
> else would need to change to make that okay?
I'm not sure I understand why this would warrant "long-term solution" or a
FIXME. This code handles an existing feature that just happens to be a corner
case from the dependency scanning point of view. (You can read up on the
feature [[ https://clang.llvm.org/docs/Modules.html | here ]].)
What do you mean by encoding the module map redundantly?
================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:275-278
+ // We don't have a good way to determine which module map described the
+ // anti-dependency (let alone what's the corresponding top-level module
+ // map). We simply specify all the module maps in the order they were
loaded
+ // during the implicit build during scan.
----------------
dexonsmith wrote:
> Is there a FIXME to leave behind for tracking the anti-dependencies?
That's a good idea. I left a FIXME in D120464 to specifically serialize the
information on anti-dependency introducing module maps here: D120464, but it
would make sense to mention it here too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120465/new/
https://reviews.llvm.org/D120465
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
