a.sidorin requested changes to this revision. a.sidorin added a subscriber: aaron.ballman. a.sidorin added a comment. This revision now requires changes to proceed.
Hello Kareem, Thank you for working on it! You can find my comments below. ================ Comment at: include/clang/ASTMatchers/ASTMatchers.h:5459 +/// \endcode +const internal::VariadicDynCastAllOfMatcher<Expr, CXXDependentScopeMemberExpr> + cxxDependentScopeMemberExpr; ---------------- I'd prefer to have a discussion with ASTMatcher developers (@aaron.ballman, @klimek) first. Maybe it is better to place this matcher into ASTImporterTest.cpp - as I understand, it will be used very rarely. ================ Comment at: lib/AST/ASTImporter.cpp:3495 + return nullptr; + if (!DC) + return nullptr; ---------------- If `DC` is `nullptr`, we should return on the previous line - `ImportDeclParts` returns `true` in this case. We may turn it into an assertion, however. ================ Comment at: lib/AST/ASTImporter.cpp:3509 + + return FunctionTemplateDecl::Create(Importer.getToContext(), DC, Loc, + Name, Params, ToDecl); ---------------- Seems like we don't attempt to lookup an existing declaration first. This may lead to appearance of multiple definitions of same decl. I'd also prefer to see a functional test for this in test/ASTMerge. You can also refer to https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp#L3594 because some important parts (setting of lexical decl context, adding a decl to the imported map) are missed here. https://reviews.llvm.org/D26904 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits