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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
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);
----------------
vsapsai wrote:
> 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.
Oh, good point. I think NameAsWritten is what we're looking for here. Thanks!


================
Comment at: clang/lib/Frontend/DependencyFile.cpp:94
+  void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem) override {
+    if (!llvm::sys::path::is_absolute(HeaderPath))
+      return;
----------------
bruno wrote:
> Can you add a testecase for when `HeaderPath` isn't absolute? 
Sure, new patch adds a test. This was meant to avoid including dependencies found in -fmodule-files, but the new patch does this more directly by adding a `bool Imported` parameter to this function. I sprinkled some comments around explaining.


https://reviews.llvm.org/D53522