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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1169
+  MatchVerifier<Decl> Verifier;
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(has(fieldDecl()))));
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl()))));
----------------
a.sidorin wrote:
> Tests with `has(fieldDecl())` duplicate tests with `hasFieldOrder()`. Is it intentional?
Ok, I removed the redundant `has(fieldDecl())` checks.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1171
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl()))));
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"}))));
+  EXPECT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"}))));
----------------
a.sidorin wrote:
> Should this be `match(From, ...)` instead of `To`?

That's right, good catch.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1177
+    ASTImporterTestBase,
+    DISABLED_CXXRecordDeclFieldsShouldBeInCorrectOrderEvenWhenWeImportFirstTheLastDecl) {
+  Decl *From, *To;
----------------
a.sidorin wrote:
> This identifier is very long. How about renaming it to DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder? It is 60 char long instead of 82.
Ok, I like the shorter name, changed it.


Repository:
  rC Clang

https://reviews.llvm.org/D43967