[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: unittests/AST/ASTImporterTest.cpp:276
+    // This will not create the file more than once.
+    createVirtualFile(ToAST.get(), It->FileName, It->Code);
+
----------------
xazax.hun wrote:
> Maybe an IfNeeded suffix or something like that rather than a comment?
Added the suffix.


================
Comment at: unittests/AST/ASTImporterTest.cpp:289
+    for (auto &Tu : FromTUs) {
+      if (Tu.Unit) {
+        llvm::errs() << "FromAST:\n";
----------------
xazax.hun wrote:
> When can the TU.Unit be nullptr here?  Should this be an assert instead?
Good catch, changed it.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1045
+
+  assert(Check(From));
+  Check(To);
----------------
xazax.hun wrote:
> I wonder why we only assert on the first one and if we should use GTEST macros here. Same questions below.
Changed to use GTEST macros.
The reason why we assert only on the first is this: In the first check we verify the original "from" AST, if the assumption we made on the original AST is false then there is no point to continue. However, in the second check we do verify the logic of the importer which may fail. But that is not a fatal failure, because if there were any other checks after that we would like to know if those checks pass or not.
The main difference between `EXPECT_*` and `ASSERT_*` is that `ASSERT_*` will terminate the given test if it fails, but `EXPECT_*` keep on going and subsequent checks will be executed in the same test.



Repository:
  rC Clang

https://reviews.llvm.org/D43967