Thank you for your comments, I’m working on them now. Sean
> On May 11, 2016, at 4:39 AM, Aleksei Sidorin <a.sido...@samsung.com> wrote: > > a.sidorin added a comment. > > I' d like to have some tests for this. CXXCtorInitializers can be tested with > D14224 <http://reviews.llvm.org/D14224>-like stuff or with ASTMatchers > (ASTImporterTest.cpp). > Some code samples may be found in CXX/Sema tests, you just need to > port/simplify them. > > > ================ > Comment at: lib/AST/ASTImporter.cpp:3034 > @@ +3033,3 @@ > + SmallVector<CXXCtorInitializer *, 4> CtorInitializers; > + for (const CXXCtorInitializer *I : FromConstructor->inits()) { > + CXXCtorInitializer *ToI =cast_or_null<CXXCtorInitializer>( > ---------------- > In my latest patch, I have introduced a function named > `ImportContainerChecked()`. I think it could make this code a bit more clean. > What do you think? > > ================ > Comment at: lib/AST/ASTImporter.cpp:3035 > @@ +3034,3 @@ > + for (const CXXCtorInitializer *I : FromConstructor->inits()) { > + CXXCtorInitializer *ToI =cast_or_null<CXXCtorInitializer>( > + Importer.Import(I)); > ---------------- > Space after '=' > > ================ > Comment at: lib/AST/ASTImporter.cpp:3041 > @@ +3040,3 @@ > + } > + if (unsigned NumInitializers = CtorInitializers.size()) { > + CXXCtorInitializer **Memory = new (Importer.getToContext()) > ---------------- > If we move this condition (I suggest to use > `FromConstructor->getNumCtorIintializers()` instead) upper to cover the > importing loop, we may skip this loop as well. > > ================ > Comment at: lib/AST/ASTImporter.cpp:3045 > @@ +3044,3 @@ > + std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory); > + > llvm::cast<CXXConstructorDecl>(ToFunction)->setCtorInitializers(Memory); > + llvm::cast<CXXConstructorDecl>(ToFunction)->setNumCtorInitializers( > ---------------- > As I can see, LLVM code style avoids qualified `cast`s. > > ================ > Comment at: lib/AST/ASTImporter.cpp:6384 > @@ +6383,3 @@ > + return nullptr; > + > + return new (ToContext) > ---------------- > Trailing whitespace. > > ================ > Comment at: lib/AST/ASTImporter.cpp:6389 > @@ +6388,3 @@ > + Import(From->getRParenLoc()), > + From->isPackExpansion() ? > + Import(From->getEllipsisLoc()) : SourceLocation()); > ---------------- > The indentation for `?:` here is a bit confusing. > > ================ > Comment at: lib/AST/ASTImporter.cpp:6396 > @@ +6395,3 @@ > + return nullptr; > + > + return new (ToContext) > ---------------- > Training whitespace. It's pretty difficult to find them with Phabricator, > could you check your patch for them? > > ================ > Comment at: lib/AST/ASTImporter.cpp:6423 > @@ +6422,3 @@ > + } else { > + return nullptr; > + } > ---------------- > It seems like a case with indexed initializer is missed here. You can find my > implementation on > https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp#L2456-L2464. > A sample that requires it is `auto parens4 = [p4(1)] {};` > (test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p4.cpp) > > > > Repository: > rL LLVM > > http://reviews.llvm.org/D20118 > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
