[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Gabor!

Thank you for this patch! Do you plan to enable this functionality for AST import checking?
Some comments are inline.



================
Comment at: unittests/AST/Language.h:1
+//===- unittest/AST/Language.h - AST unit test support ---------------===//
+//
----------------
Header line is too short.


================
Comment at: unittests/AST/Language.h:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
Could you please add a brief file description?


================
Comment at: unittests/AST/Language.h:13
+
+#include <vector>
+#include <string>
----------------
System includes should follow LLVM includes.


================
Comment at: unittests/AST/Language.h:37
+
+inline ArgVector getBasicRunOptionsForLanguage(Language Lang) {
+  ArgVector BasicArgs;
----------------
I think this function is too big for a header. Same below. Could you create a source file?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:2
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTImporter.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
----------------
Do we need ASTImporter.h?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:39
+    auto getDecl = [](ASTContext &Ctx, const std::string &Name) -> NamedDecl * {
+      IdentifierInfo *ImportedII = &Ctx.Idents.get(Name);
+      assert(ImportedII && "Declaration with the identifier "
----------------
s/Import/Search?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:47
+
+      // We should find one Decl but one only
+      assert(FoundDecls.size() > 0);
----------------
Nit: comments should end with dot. Could you please check?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:48
+      // We should find one Decl but one only
+      assert(FoundDecls.size() > 0);
+      assert(FoundDecls.size() < 2);
----------------
Can we `assert(FoundDecls.size() == 1)`?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:54
+
+    NamedDecl *d0 = getDecl(Ctx0, Identifier);
+    NamedDecl *d1 = getDecl(Ctx1, Identifier);
----------------
`D0`, `D1` (capital). Same below.


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:73
+  auto t = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
----------------
Could we just overload testStructuralMatch to accept tuple or pair?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:119
+      *cast<ClassTemplateDecl>(get<1>(t))->spec_begin();
+  ASSERT_TRUE(Spec0 != nullptr);
+  ASSERT_TRUE(Spec1 != nullptr);
----------------
Should we assert for `spec_begin() != spec_end()` instead or within these assertions?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:163
+TEST_F(StructuralEquivalenceTest, DISABLED_WrongOrderInNamespace) {
+  auto Code0 =
+      R"(
----------------
There is no Code1, so I think we can call it just "Code". Same below.


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:174
+      )";
+  auto t = makeNamedDecls( Code0, Code0, Lang_CXX);
+
----------------
1. It's better to use more meaningful names than `t`. DeclTuple?
2. The space after `(` is redundant.


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:193
+  auto Code0 = "class X { int a; int b; };";
+  auto t = makeNamedDecls( Code0, Code0, Lang_CXX, "X");
+
----------------
Redundant space. Could you clang-format?


================
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:195
+
+  ASSERT_TRUE(get<0>(t) != nullptr);
+  ASSERT_TRUE(get<1>(t) != nullptr);
----------------
These assertions are always true because there is a strong C assertion in the callee.


Repository:
  rC Clang

https://reviews.llvm.org/D46867