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

Reply via email to