[PATCH] Implement CapturedStmt AST

Doug Gregor dgregor at apple.com
Wed Feb 6 06:23:57 PST 2013



================
Comment at: include/clang/AST/Stmt.h:1892
@@ +1891,3 @@
+public:
+  // LambdaCaptureKind and this enumerator need to be extended
+  // to handle other capture kinds.
----------------
I don't understand the comment here. What other capture kinds do you expect here or in lambdas?

Additionally, it seems somewhat strange that we're using LambdaCaptureKind to communicate the capture kind with clients, when a CapturedStmt is not a lambda. Shouldn't CapturedStmt have its own capture kind enum?

Finally, do you need by-copy captures? I would have expected by-reference and "this" captures only, and by-copy captures introduce a lot of baggage. See, for example, the array-indices stuff in CXXLambdaExpr::Capture.

================
Comment at: include/clang/AST/Stmt.h:2007
@@ +2006,3 @@
+                   const Capture *end);
+
+  SourceLocation getLocStart() const LLVM_READONLY {
----------------
Please use ArrayRef<Capture*> rather than the begin/end pair.

================
Comment at: include/clang/AST/Stmt.h:2024
@@ +2023,3 @@
+    return child_range(&SubStmt, &SubStmt + 1);
+  }
+};
----------------
If you're going to have by-copy captures, then this children() function isn't complete, because it doesn't cover the copy expressions for by-copy captures. 

================
Comment at: lib/AST/Stmt.cpp:1055
@@ +1054,3 @@
+    // extended to capture non-auto variables.
+    if (I->getCapturedVar() == variable)
+      return true;
----------------
I don't understand this comment. Variables with static storage duration don't ever need to be captured. Are you referring to thread-private variables here?

================
Comment at: lib/Sema/TreeTransform.h:9365
@@ +9364,3 @@
+  return Owned(S);
+}
+
----------------
llvm_unreachable("Not implemented yet") is a good way to make sure nobody gets surprised by this.

================
Comment at: lib/Serialization/ASTReaderStmt.cpp:332
@@ -327,1 +331,3 @@
+}
+
 void ASTStmtReader::VisitExpr(Expr *E) {
----------------
Again, llvm_unreachable is a good way to avoid surprises later.

================
Comment at: lib/Serialization/ASTWriterStmt.cpp:263
@@ +262,3 @@
+  Code = serialization::STMT_CAPTURED;
+}
+
----------------
Same llvm_unreachable comment.


http://llvm-reviews.chandlerc.com/D370



More information about the cfe-commits mailing list