[PATCH] D61786: [ASTImporter] Separate unittest files

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

Hi Gabor,
I like this change! LGTM, just a few nits.



================
Comment at: clang/unittests/AST/ASTImporterGenericRedeclTest.cpp:20
+
+// FIXME put these structs and the tests rely on them into their own separate
+// test file!
----------------
Is this FIXME obsolete now?


================
Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:37
+// FunctionDecl:
+auto *ExternF = "void f();";
+auto *StaticF = "static void f();";
----------------
Can we use StringRef (or, at least `const auto *`)?


================
Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:49
+    ::testing::WithParamInterface<std::tuple<ArgVector, const char *>>;
+// Fixture to test the redecl chain of Decls with the same visibility.  Gtest
+// makes it possible to have either value-parameterized or type-parameterized
----------------
Double spaces in the end of sentences look a bit strange.


================
Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:139
+
+    auto *ToF0 = FirstDeclMatcher<DeclTy>().match(ToTu, getPattern());
+    auto *FromF1 = FirstDeclMatcher<DeclTy>().match(FromTu, getPattern());
----------------
Optional: F0/F1 (where F stands for "function", I guess) can be replaced with D0/D1 (for decl) since the code is generic.


================
Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:157
+    TranslationUnitDecl *FromTu1 = getTuDecl(getCode1(), Lang_CXX, "input1.cc");
+    auto *FromF0 =
+        FirstDeclMatcher<DeclTy>().match(FromTu0, getPattern());
----------------
This should fit a single line.


================
Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:190
+
+bool ExpectLink = true;
+bool ExpectNotLink = false;
----------------
const?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61786/new/

https://reviews.llvm.org/D61786