[PATCH] D53522: [Frontend] Include module map header declaration in dependency file output

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Have a few comments but didn't try to come up with edge cases yet.



================
Comment at: clang/include/clang/Lex/ModuleMap.h:649-650
+  ///        This can differ from \c Header's name due to symlinks.
   void addHeader(Module *Mod, Module::Header Header,
-                 ModuleHeaderRole Role, bool Imported = false);
+                 ModuleHeaderRole Role, StringRef OrigHeaderName = "",
+                 bool Imported = false);
----------------
How `OrigHeaderName` is different from `Module::Header.NameAsWritten`? Based on the names they should be the same, curious if and when it's not the case.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1200-1201
+
+    SmallString<128> FullPath(Mod->Directory->getName());
+    llvm::sys::path::append(FullPath, OrigHeaderName);
+
----------------
Would it be better to build `FullPath` outside of the loop?


================
Comment at: clang/test/Modules/dependency-file-symlinks.c:20-21
+
+// Run clang again, to make sure that we get identical output with the cached
+// modules.
+
----------------
That's neat. I might steal this approach for my own changes.


Repository:
  rC Clang

https://reviews.llvm.org/D53522