[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: lib/Basic/SourceManager.cpp:1626
+    if (FileContentCache->OrigEntry == SourceFile) {
+      if (Callback(FileID::get(I)))
+        return true;
----------------
Should we check `FileID::get(I)` is valid?


================
Comment at: lib/Basic/SourceManager.cpp:1628
+        return true;
+    } else if (LookForFilesystemUpdates &&
+               (SourceFileName || (SourceFileName = llvm::sys::path::filename(
----------------
The conditions here are pretty hard to follow and reason about. Could we maybe split them (some documentation would also help)?


================
Comment at: lib/Basic/SourceManager.cpp:1628
+        return true;
+    } else if (LookForFilesystemUpdates &&
+               (SourceFileName || (SourceFileName = llvm::sys::path::filename(
----------------
ioeric wrote:
> The conditions here are pretty hard to follow and reason about. Could we maybe split them (some documentation would also help)?
In the original version, file system updates are checked last (after modules). Any reason to move it before modules? 

Also, it seems that this code path could also be run when `FileID`above is invalid? So I wonder whether `else if` is correct here.


https://reviews.llvm.org/D50926