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

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Gabor! This looks much better.



================
Comment at: unittests/AST/ASTImporterTest.cpp:1000
+      R"(
+      template<typename _T>
+      struct X {};
----------------
In C++, names started with underscore are reserved for implementation so it's undesirable to use them.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1003
+
+      void declToImport(int y,X<int> &x){}
+
----------------
I still can see a number of style violations in inline code. Some examples: lack of space between arguments and parameters, no space before '{' in `g(){`, etc. Could you please fix them?


================
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()))));
----------------
Tests with `has(fieldDecl())` duplicate tests with `hasFieldOrder()`. Is it intentional?


================
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"}))));
----------------
Should this be `match(From, ...)` instead of `To`?


================
Comment at: unittests/AST/ASTImporterTest.cpp:1177
+    ASTImporterTestBase,
+    DISABLED_CXXRecordDeclFieldsShouldBeInCorrectOrderEvenWhenWeImportFirstTheLastDecl) {
+  Decl *From, *To;
----------------
This identifier is very long. How about renaming it to DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder? It is 60 char long instead of 82.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1194
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl()))));
+  ASSERT_TRUE(
+      Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
----------------
Same as upper.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1211
+  MatchVerifier<Decl> Verifier;
+  // matches the implicit decl
+  auto Matcher = classTemplateDecl(has(cxxRecordDecl(has(cxxRecordDecl()))));
----------------
Please make it a complete sentence.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1258
+      Lang_CXX);
+  Decl* From = LastDeclMatcher<Decl>{}.match(FromTU, functionDecl());
+  const Decl* To = Import(From, Lang_CXX);
----------------
Please stick `*` to the name (below too).


Repository:
  rC Clang

https://reviews.llvm.org/D43967