[PATCH] D20118: Add support for injected class names and constructor initializers in C++

[Sean Callanan via cfe-commits]

Thank you for your comments, I’m working on them now.

Sean

> On May 11, 2016, at 4:39 AM, Aleksei Sidorin <a.sidorin at 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
> 
> 
>