[PATCH] D14286: ASTImporter: expressions, pt.1

[Sean Callanan via cfe-commits]

spyffe requested changes to this revision.
spyffe added a comment.
This revision now requires changes to proceed.

Overall this is a great improvement and I look forward to taking advantage of this patch in LLDB!

I had one specific nit about `NullPtrTy`, and one more general comment about your infrastructure that imports multiple things – it seems like we can collaborate and create some combination of `ImportArray` and your code to handle all cases.  Particularly you handle the cases where the array doesn't have to be  allocated inside `getToContext()` – my code doesn't handle that at all.

Thanks for working on this.


================
Comment at: lib/AST/ASTImporter.cpp:35
@@ +34,3 @@
+    void ImportMultipleItems(IIter Ibegin, IIter Iend, OIter Obegin) {
+      ASTImporter &ImporterRef = Importer;
+      std::transform(Ibegin, Iend, Obegin,
----------------
I recently added a function, `ImportArray`, that does something similar.  Could it be adapted to your needs?

================
Comment at: lib/AST/ASTImporter.cpp:5462
@@ +5461,3 @@
+  ASTContext &ToCtx = Importer.getToContext();
+  return new(ToCtx) CXXNullPtrLiteralExpr(ToCtx.NullPtrTy,
+                                          Importer.Import(E->getLocation()));
----------------
I'm worried about using `NullPtrTy` here directly, because the type may have been coerced to a `typedef` or something like that.  The fact that the constructor takes a type argument suggests that the type is not implicit – I'd feel much more comfortable just importing the type.

================
Comment at: lib/AST/ASTImporter.cpp:5466
@@ +5465,3 @@
+
+Expr *ASTNodeImporter::VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E) {
+  ASTContext &ToCtx = Importer.getToContext();
----------------
I recently committed `VisitCXXBoolLiteralExpr`.  Sorry to step on your toes!

================
Comment at: lib/AST/ASTImporter.cpp:5878
@@ -5346,1 +5877,3 @@
 
+Expr *ASTNodeImporter::VisitCXXThisExpr(CXXThisExpr *E) {
+  QualType T = Importer.Import(E->getType());
----------------
Sorry again!  I recently committed `VisitCXXThisExpr`

================
Comment at: unittests/AST/ASTImporterTest.cpp:118
@@ +117,3 @@
+                                   hasType(
+                                     asString("const char [4]")))))))));
+  EXPECT_TRUE(testImport("void declToImport() { L\"foo\"; }",
----------------
This is a good point; if you use LLDB to test this (e.g., lldb's `test/testcases/expression_command/top-level`) then you can verify that these imported entities make it all the way to generated machine code and work as expected.

Ideally there would be a mode in Clang that parses the input file, makes a translation unit out of it, imports everything from that translation unit into another translation unit, and then `CodeGen`s the second translation unit.  That is something I'd like to hook up when I get some time.  I've also talked to Doug Gregor and he suggested that perhaps .pch files can be abused for this purpose, repurposing some of the functionality in the `ASTMerge` tests (though those don't do `CodeGen`).


http://reviews.llvm.org/D14286