a.sidorin added a comment. Hello Peter,
Looks mostly good, but there are some minor comments. ================ Comment at: lib/AST/ASTImporter.cpp:5500 + + TemplateArgumentListInfo ToTAInfo; + TemplateArgumentListInfo *ResInfo = nullptr; ---------------- xazax.hun wrote: > szepet wrote: > > xazax.hun wrote: > > > According to phabricator this code is very similar to a snippet starting > > > from line 4524 and some code bellow. Maybe it would be worth to have a > > > function instead of duplicating? > > Good point, I would do it in a separate patch and add it as a dependency if > > it is OK. > Sure, that would be awesome. :) This is a template I have prepared for a patch still non-commited yet: ``` template<typename InContainerTy> bool ImportTemplateArgumentListInfo(const InContainerTy &Container, TemplateArgumentListInfo &ToTAInfo) { for (const auto &FromLoc : Container) { if (auto ToLoc = ImportTemplateArgumentLoc(FromLoc)) ToTAInfo.addArgument(*ToLoc); else return true; } return false; } ``` ================ Comment at: lib/AST/ASTImporter.cpp:5626 + DeclarationName Name = Importer.Import(E->getName()); + if(!E->getName().isEmpty() && Name.isEmpty()) + return nullptr; ---------------- Needed space after if. ================ Comment at: unittests/AST/ASTImporterTest.cpp:574 + "template <typename T> void declToImport() {" + "S<T>::foo;" + "}", ---------------- Could you please align the code as conventions require? https://reviews.llvm.org/D38845 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits