[PATCH] D26904: [astimporter] Support importing CXXDependentScopeMemberExpr and FunctionTemplateDecl

[Aleksei Sidorin via cfe-commits]

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