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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: lib/Basic/SourceManager.cpp:1626
+    if (FileContentCache->OrigEntry == SourceFile) {
+      if (Callback(FileID::get(I)))
+        return true;
----------------
ioeric wrote:
> Should we check `FileID::get(I)` is valid?
That's not really necessary. The FileID we get should be valid as a local SLoc entry should have a corresponding FileID. The SLoc 0 is not a File loc entry so we can never get FileID::get(0) here.


================
Comment at: lib/Basic/SourceManager.cpp:1628
+        return true;
+    } else if (LookForFilesystemUpdates &&
+               (SourceFileName || (SourceFileName = llvm::sys::path::filename(
----------------
ioeric wrote:
> 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.
I just realized that the original file system code has never been executed and was just dead code! If we take a look at this logic:

```
for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
    FileID IFileID;
    IFileID.ID = I;
    const SLocEntry &SLoc = getSLocEntry(IFileID, &Invalid);
    if (Invalid)
      return false;
```

You'll notice that the loop starts iterating at `0`. Get SLocEntry is called with FileID `0`, which sets the `Invalid` flag to true. Then we simply return, so the loop never reached the code below. Looks like it's a regression that happened years ago. I removed this code for now, but I'll reinstate it correctly in a follow-up patch.



Repository:
  rC Clang

https://reviews.llvm.org/D50926