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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Coming back to this one, I see a failing test: PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble

Note that PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble references the header paths in different ways ("//./header1.h" vs "//./foo/../header1.h"). Before this change, this was not a problem because OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change, the key is the file path.

Is there a nice way to map different file paths of the same file to the same id without touching the real file system? Would it be sufficient to normalize the file paths? If yes, is there a utility function for this (can't find it right now).



================
Comment at: include/clang/Frontend/ASTUnit.h:193
   /// some number of calls.
-  unsigned PreambleRebuildCounter;
+  unsigned PreambleRebuildCountdownCounter;
+
----------------
ilya-biryukov wrote:
> NIT: Maybe shorten to `PreambleRebuildCountdown`?
> It's not a great name, but a bit shorter than now and seems to convey the same meaning.
Will do.


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


================
Comment at: unittests/Frontend/PCHPreambleTest.cpp:133
+
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
----------------
ilya-biryukov wrote:
> 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);
> ```
> Are we testing the right thing here?
Huch, indeed, the test is wrong. 

I'll incorporate your suggested test (real file is added after providing unsaved file) and add also another one: real file is removed after providing an unsaved file.

> That is actually a bug, we should rebuild the preamble in that case.
Agree, the preamble should be rebuild in such a case, but that's something for a separate change.



Repository:
  rC Clang

https://reviews.llvm.org/D41005