[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Frontend/ASTUnit.h:196
+  /// \brief Counter indicating how often the preamble was build in total.
+  unsigned PreambleCounter;
+
----------------
nik wrote:
> Any better name for this one? Otherwise I would suggest renaming PreambleRebuildCounter to PreambleRebuildCountdownCounter to make them more distinct.
+1, names PreambleRebuildCounter and PreambleCounter are too similar.



================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
 
+  vfs::OverlayFileSystem OFS(VFS);
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> IMFS(
----------------
nik wrote:
> ilya-biryukov wrote:
> > Can we do the same thing without creating an additional `OverlayFileSystem`?
> > 
> > If we add a separate map to check for those:
> > ```
> > llvm::StringMap<PreambleFileHash> OverriddenFilesWithoutFS.
> > // Populate the map with paths not existing in VFS.
> > 
> > for (const auto &F : FilesInPreamble) {
> >     vfs::Status Status;
> >     if (!moveOnNoError(OFS.status(F.first()), Status)) {
> >       // If we can't stat the file, check whether it was overriden
> >       auto It = OverriddenFilesWithoutFS[F.first()];
> >       if (It == OverriddenFilesWithoutFS.end())
> >         return false;  
> >       //.....
> >     }
> > //......
> > 
> > }
> > ```
> > Can we do the same thing without creating an additional OverlayFileSystem?
> 
> Hmm, I thought I had a good reason for this one. I don't remember it and the test still passes, so I did it without it now.
> 
> Is there any real advantage in first trying the stat, then checking OverriddenFilesWithoutFS? Since I don't see any, I've changed order because the stat can then be avoided; also, it removes some nesting.
I don't see any advantage.
I used to think it had something to do with overriden symlinks, but after thinking more about it I'm not even sure what should the semantics of those be. And I don't think the current code handles that (we have neither tests nor comments suggesting that this use-case was considered in the first place).

This possibly handles case-insensitive filesystems, like NTFS on Windows. But I'm not sure if that was the original intention of this code.


================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:461
 
     std::map<llvm::sys::fs::UniqueID, PreambleFileHash>::iterator Overridden =
         OverriddenFiles.find(Status.getUniqueID());
----------------
Will anything fail if we remove the map from `UniqueID` to hashes of overriden files and the corresponding checks?

We should either document why having `UniqueID`-based checks is important or remove the code doing those checks.


Repository:
  rC Clang

https://reviews.llvm.org/D41005