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

[Aleksei Sidorin via cfe-commits]

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