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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
