[PATCH] D20137: [PCH] Fixed bugs with preamble invalidation when files change (on Windows)

[Cameron via cfe-commits]

cameron314 added inline comments.

================
Comment at: lib/Basic/FileManager.cpp:304-307
@@ -303,1 +303,6 @@
 
+  if (UFE.isVirtual()) {
+    UFE.Name = InterndFileName;
+    return &UFE;
+  }
+
----------------
rsmith wrote:
> It looks like this is unreachable: `IsVirtual` is only ever `true` when `IsValid` is also `true`, and we don't reach this line if `UFE.isValid()`. As this is the only consumer of `FileEntry::IsValid`, it looks like it does nothing. Am I missing something?
Hmm, interesting, good catch. Back when I made this change UFE.IsValid was //not// set to true in the fall-through after this if, which is why I had to introduce `IsVirtual` in the first place.

But looking at the history on this file, it seems it //was// set to true since the very beginning. I think we'd removed it in our local fork at one point to work around a different bug, but it's back to normal in ours as well. So I guess this part of the patch is unnecessary now. Yay! I'll remove it, it wasn't very pretty anyway.

================
Comment at: lib/Frontend/ASTUnit.cpp:1402-1406
@@ +1401,7 @@
+
+        vfs::Status Status;
+        if (FileMgr->getNoncachedStatValue(RB.first, Status)) {
+          AnyFileChanged = true;
+          break;
+        }
+
----------------
rsmith wrote:
> If there are multiple file names with the same inode, `RB.first` is an arbitrarily-chosen one of those names, and stat'ing it isn't sufficient to check that none of the other names for the file have changed. Maybe we need to store a list of filenames used for each `UniqueID`?
I'm not sure I understand -- if the file changed on disk, why would it matter which name we're accessing it by?


http://reviews.llvm.org/D20137