[PATCH] D134923: [clang][deps] Canonicalize module map path

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D134923#3825673 <https://reviews.llvm.org/D134923#3825673>, @jansvoboda11 wrote:

> IIUC this not only canonicalizes path to the main input file when compiling a PCM, but also `-fmodule-map-file=` arguments for dependencies, correct? Since that behavior is desirable, I think it would make sense to check for that in the test, WDYT?

Yep, makes sense to me!



================
Comment at: clang/lib/Lex/ModuleMap.cpp:1307
+  // Do not canonicalize within the framework; the module map parser expects
+  // Modules/ not Versions/A/Modules.
+  if (llvm::sys::path::filename(Dir) == "Modules") {
----------------
jansvoboda11 wrote:
> Is that because the parser is looking for headers in `../Headers` and `../PrivateHeaders`? If so, would it make sense to make the parser smarter and canonicalize even framework paths here?
The failure I am aware of is that `loadModuleMapFile` will not detect the correct module `Directory`.  I'm not sure if there are more failures after that.  I didn't want to complicate this logic, because it seems like there are several possibilities:

* Modules/module.modulemap
* Versions/A/Modules/module.modulemap
* Versions/Current/Modules/module.modulemap

Also, if we get one of the versioned paths, but `Modules/module.modulemap` does not point to that specific file then it would be incorrect to use it.  I'm inclined to leave this alone, at least for the current change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134923