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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/Frontend/DependencyFile.cpp:100
+
+    DepCollector.maybeAddDependency(HeaderPath, /*FromModule=*/false, IsSystem,
+                                    /*IsModuleFile*/ false,
----------------
This `/*FromModule=*/false` looks confusing and suspicious but seems like we don't use this argument, so it doesn't really matter and worth cleaning up (out of scope for this change).


================
Comment at: clang/lib/Frontend/DependencyFile.cpp:105-109
+  void moduleMapAddUmbrellaHeader(FileManager *FileMgr, const FileEntry *Header,
+                                  bool IsSystem, bool Imported) override {
+    DepCollectorMMCallbacks::moduleMapAddHeader(Header->getName(), IsSystem,
+                                                Imported);
+  }
----------------
What is the purpose of this method and the one in `DFGMMCallback`? It looks correct-ish but after removing this callback, the tests are still passing.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1198-1203
+    // If the header in the module map refers to a symlink, Header.Entry
+    // refers to the actual file. The callback should be notified of both.
+    if (!FullPathAsWritten.empty() &&
+        !FullPathAsWritten.equals(Header.Entry->getName())) {
+      Cb->moduleMapAddHeader(FullPathAsWritten, Mod->IsSystem, Imported);
+    }
----------------
It is strange but after removing this part the tests are still passing. I suspect the code is correct but the test allows some roundabout way to add symlink to dependencies. In my experiments only `DFGMMCallback::moduleMapAddHeader` affects the tests. Is it expected?


https://reviews.llvm.org/D53522