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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.



================
Comment at: unittests/AST/ASTImporterTest.cpp:143
 
+class Fixture : public ::testing::Test {
+
----------------
I do not like the name of this class. It is like calling a variable as `variable`. 


================
Comment at: unittests/AST/ASTImporterTest.cpp:148
+
+  //Buffers for the contexts, must live in test scope
+  std::string ToCode;
----------------
Missing period on the end of the comment and missing space at the beginning.


================
Comment at: unittests/AST/ASTImporterTest.cpp:163
+  // We may have several From context and related translation units.
+  std::list<TU> FromTUs;
+
----------------
I think it might worth to have a note why do we use a list and not a vector.


================
Comment at: unittests/AST/ASTImporterTest.cpp:170
+    ASTContext &ToCtx = ToAST->getASTContext();
+    vfs::OverlayFileSystem *OFS = static_cast<vfs::OverlayFileSystem *>(
+        ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
----------------
Repeated type, use auto. Same below.
Are we sure these cast's will not fail? Shouldn't we use dynamic casts and asserts?


================
Comment at: unittests/AST/ASTImporterTest.cpp:184
+  // Must not call more than once within the same test.
+  std::tuple<Decl *, Decl *>
+  getImportedDecl(StringRef FromSrcCode, Language FromLang,
----------------
I wonder if pair or tuple is the better choice here. I have no strong preference just wondering.


================
Comment at: unittests/AST/ASTImporterTest.cpp:187
+                  StringRef ToSrcCode, Language ToLang,
+                  const char *const Identifier = "declToImport") {
+    ArgVector FromArgs, ToArgs;
----------------
Maybe StringRef for the Identifier? 


================
Comment at: unittests/AST/ASTImporterTest.cpp:190
+    FromArgs = getBasicRunOptionsForLanguage(FromLang);
+    ToArgs = getBasicRunOptionsForLanguage(ToLang);
+
----------------
Maybe declaring in the same line would be better.


================
Comment at: unittests/AST/ASTImporterTest.cpp:227
+    assert(
+        std::find_if(FromTUs.begin(), FromTUs.end(), [&FileName](const TU &e) {
+          return e.FileName == FileName;
----------------
I prefer to capture FileName by value. Param names should be upper case. Same below.


================
Comment at: unittests/AST/ASTImporterTest.cpp:231
+
+    ArgVector Args= getBasicRunOptionsForLanguage(Lang);
+    FromTUs.emplace_back(SrcCode, FileName, Args);
----------------
Formatting is off here.


================
Comment at: unittests/AST/ASTImporterTest.cpp:253
+    auto It =
+        std::find_if(FromTUs.begin(), FromTUs.end(), [&From](const TU &e) {
+          return e.TUDecl == From->getTranslationUnitDecl();
----------------
Capture From as value.


================
Comment at: unittests/AST/ASTImporterTest.cpp:996
+TEST_F(Fixture, TUshouldNotContainTemplatedDeclOfFunctionTemplates) {
+
+  Decl *From, *To;
----------------
Redundant new lines, same in most of the tests' beginning.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1006
+    for (auto Child : TU->decls()) {
+      if (FunctionDecl *FD = dyn_cast<FunctionDecl>(Child)) {
+        if (FD->getNameAsString() == "declToImport") {
----------------
Repeated type.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1033
+    for (auto Child : TU->decls()) {
+      if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Child)) {
+        if (RD->getNameAsString() == "declToImport") {
----------------
Repeated type.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1062
+    for (auto Child : TU->decls()) {
+      if (TypeAliasDecl *AD = dyn_cast<TypeAliasDecl>(Child)) {
+        if (AD->getNameAsString() == "declToImport") {
----------------
Repeated type.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1091
+
+  // Check that the ClassTemplateSpecializationDecl is NOT the child of the TU
+  auto Pattern =
----------------
Missing periods in comments.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1156
+    for (auto Child : cast<DeclContext>(D)->decls()) {
+      if (FieldDecl *FD = dyn_cast<FieldDecl>(Child)) {
+        if (FD->getNameAsString() != FieldNamesInOrder[i++]) {
----------------
Repeated type.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1198
+    for (auto Child : cast<DeclContext>(D)->decls()) {
+      if (FieldDecl *FD = dyn_cast<FieldDecl>(Child)) {
+        if (FD->getNameAsString() != FieldNamesInOrder[i++]) {
----------------
Repeated type.


Repository:
  rC Clang

https://reviews.llvm.org/D43967