[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:193
   /// some number of calls.
-  unsigned PreambleRebuildCounter;
+  unsigned PreambleRebuildCountdownCounter;
+
----------------
NIT: Maybe shorten to `PreambleRebuildCountdown`?
It's not a great name, but a bit shorter than now and seems to convey the same meaning.


================
Comment at: unittests/Frontend/PCHPreambleTest.cpp:130
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, "#include \"//./header1.h\"\n"
+                    "int main() { return ZERO; }");
----------------
NIT: Maybe use raw string literals? Escpated string are hard to read.
E.g., 

```R"cpp(
#include "//./header1.h"
int main() { return ZERO; }
)cpp"
```


================
Comment at: unittests/Frontend/PCHPreambleTest.cpp:133
+
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
----------------
Are we testing the right thing here?

We're testing that preamble does not get rebuild when some header that was previously unresolved when seen inside `#include` directive is now added to the filesystem. That is actually a bug, we should rebuild the preamble in that case.

We should probably call `RemapFile` before calling `ParseAST` instead and make sure it's properly resolved.
```
  AddFile(MainName, ...);
  // We don't call AddFile for the header at this point, so that it does not exist on the filesystem.
  RemapFile(Header1, "#define ZERO 0\n");

  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
  // Also assert that there were no compiler errors? (I.e. file was resolved properly)
  // .... 

  // Now the file is on the filesystem, call reparse and check that we reused the preamble.
  AddFile(Header1, "#define ZERO 0\n");
  ASSERT_TRUE(ReparseAST(AST));
  ASSERT_EQ(AST->getPreambleCounter(), 1U);
```


Repository:
  rC Clang

https://reviews.llvm.org/D41005