[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Gabor Marton via Phabricator via cfe-commits]

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