[PATCH] D77942: [Preamble] Invalidate preamble when missing headers become present.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

It is unfortunate that we are testing `PrecompiledPreamble` through clangd rather than its own unittest :/



================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:101
+
+  // We always receive FileNotFound followed by InclusionDirective.
+  // We want the former, so we're completely sure the file was missing.
----------------
is there a common recovery logic implemented in clang that makes this logic worth it?
because none of the existing clients seems to be implementing it.

I would rather check for `File == nullptr` in `InclusionDirective` and maybe put a fixme saying that this might still be set by a "recovering ppcallback".

Up to you though.


================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:137
+    // ...relative to the including file.
+    if (!IsAngled)
+      if (const FileEntry *IncludingFile =
----------------
nit: braces


================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:590
+  for (const auto &F : MissingFiles) {
+    // A missing file may be "provided" by an override buffer.
+    if (OverridenFileBuffers.count(F.getKey()))
----------------
i believe it can also be provided by an overridefile. maybe also check this above while going over `RemappedFiles`. i.e:

```
for(... &R : ..RemappedFiles) {
   if(!stat(R.second)) return false;
   // A missing file has been remapped into an existing file.
   if(MissingFiles.count(R.first)) return false;
 ....
}
```


================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:591
+    // A missing file may be "provided" by an override buffer.
+    if (OverridenFileBuffers.count(F.getKey()))
+      return false;
----------------
i am not sure if this works in all cases.

Assume there's a mapped buffer for "a.h", which is relative to main file.

I suppose this will be recorded with fullpath in MissingFiles, hence this lookup will fail.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77942/new/

https://reviews.llvm.org/D77942