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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
