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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 3 02:26:22 PST 2018

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.

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

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_EQ(AST->getPreambleCounter(), 1U);

  rC Clang


More information about the cfe-commits mailing list