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