[PATCH] D120465: [clang][deps] Generate necessary "-fmodule-map-file=" arguments, disable implicit module maps

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.
Herald added a project: All.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:112
 
 void dependencies::detail::collectPCMAndModuleMapPaths(
     llvm::ArrayRef<ModuleID> Modules,
----------------
Should this be renamed?


================
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);
         }
----------------
Can you clarify why this is safe to remove, even though sometimes we do need to add module map files?


================
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.
----------------
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?


================
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.
----------------
Is there a FIXME to leave behind for tracking the anti-dependencies?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120465