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