[PATCH] D26904: [astimporter] Support importing CXXDependentScopeMemberExpr and FunctionTemplateDecl
Aleksei Sidorin via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 23 01:30:56 PST 2016
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
More information about the cfe-commits
mailing list