martong added a comment.
Hi Shafik, thank you for this patch! Generally it looks quite okay to me, but I
have a few minor comments.
================
Comment at: lib/AST/ASTImporter.cpp:3049
+ // Import the body of D and attach that to FoundByLookup.
+ // This should probably be refactored into a function since we do the
+ // same below too.
----------------
Could you please do this refactor and remove this sentence from the comment?
(At line 3208 we repeat the same logic.)
================
Comment at: unittests/AST/ASTImporterTest.cpp:2243
+ auto BP = cxxMethodDecl(hasName("f"),
hasParent(cxxRecordDecl(hasName("B"))));
+ auto DP = cxxMethodDecl(hasName("f"),
hasParent(cxxRecordDecl(hasName("D"))));
+
----------------
balazske wrote:
> I think names like `BP` and `BFP ` could be better (`P` should stand for
> "Pattern" and always the last part of the name, so use `BFP` and `BFIsDefP`).
> And for storing `B::F` a `BF` can be better name than `B`.
I agree. I think the contractions we use in
`ImportOverriddenMethodTwiceOutOfClassDef` are more consistent.
================
Comment at: unittests/AST/ASTImporterTest.cpp:2325
+ EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+ EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFPIsDef), 0u);
+
----------------
balazske wrote:
> For the out-of-class definition the `hasParent(cxxRecordDecl(hasName("B")))`
> does not match? (Otherwise this count should be 1.)
That does not match, because `hasParent` matches based on the lexical decl
context. In the case of the out-of-class definition, the lexical decl context
is the global namespace (the TranslationUnitDecl).
However, the semantical decl context of the out-of-class definition is the
CXXRecordDecl with the name "B". But this relation is not symmetric. The
definition is not the child (i.e. DeclContext::containsDecl is false) of the
CXXRecordDecl rather the child of the TranslataionUnitDecl.
================
Comment at: unittests/AST/ASTImporterTest.cpp:2385
+ EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+ EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFPIsDef), 0u);
+
----------------
Perhaps it would make sense to have expectations on `DFP` and on `DFPIsDef` too.
================
Comment at: unittests/AST/ASTImporterTest.cpp:2393
+
+ // The definition should be out-of-class.
+ EXPECT_NE(ToBFInClass, ToBFOutOfClass);
----------------
Perhaps it would make sense to have expectations on `D::f() {}` too. Even
better could be to have a lambda which takes a few Decls as parameters and does
the expectations on those.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
https://reviews.llvm.org/D56936
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
