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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
