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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 2 inline comments as done.
nik added inline comments.


================
Comment at: include/clang/Frontend/PrecompiledPreamble.h:109
 
+  std::chrono::steady_clock::time_point getCreationTimePoint() const {
+    return CreationTimePoint;
----------------
ilya-biryukov wrote:
> Having this time stamp in the interface of `PrecompiledPreamble` doesn't really makes sense.
> 
> I propose we remove this method and test in a different way instead. 
> 
> For example, we could add a counter to `ASTUnit` that increases when preamble is built and check this counter.
> Or we could add a unit-test that uses `PrecompiledPreamble` directly.
> For example, we could add a counter to ASTUnit that increases when preamble is built and check this counter.

OK, I've followed this proposal because there is already test infrastructure in PCHPreambleTest that I can easily reuse.



================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
 
+  vfs::OverlayFileSystem OFS(VFS);
+  IntrusiveRefCntPtr<vfs::InMemoryFileSystem> IMFS(
----------------
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.


================
Comment at: lib/Frontend/PrecompiledPreamble.cpp:444
     vfs::Status Status;
-    if (!moveOnNoError(VFS->status(RB.first), Status))
-      return false;
-
+    assert(moveOnNoError(IMFS->status(RB.first), Status));
     OverriddenFiles[Status.getUniqueID()] =
----------------
ilya-biryukov wrote:
> `assert` macro is a no-op when `NDEBUG` is defined.
> One must never put an operation with side-effects inside `assert`.
Huch, forgot to remove it on cleanup. Anyway, it's obsolete now.


Repository:
  rC Clang

https://reviews.llvm.org/D41005