[PATCH] Implement CapturedStmt AST

Wei Pan wei.pan at intel.com
Wed Feb 6 06:48:58 PST 2013


  Thanks for reviewing!


================
Comment at: include/clang/AST/Stmt.h:1892
@@ +1891,3 @@
+public:
+  // LambdaCaptureKind and this enumerator need to be extended
+  // to handle other capture kinds.
----------------
Doug Gregor wrote:
> 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.
Agree, I will introduce a new CaptureKind enum for this.

For OpenMP, a few more capture kinds will be needed, by-reference, by-copy, by-default-constructor, etc. To make the patch cleaner, I will remove this bit.

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

================
Comment at: lib/AST/Stmt.cpp:1055
@@ +1054,3 @@
+    // extended to capture non-auto variables.
+    if (I->getCapturedVar() == variable)
+      return true;
----------------
Doug Gregor wrote:
> 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?
Yes, this comment is  for OpenMP static variable capturing. Should it be removed?

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

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

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


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



More information about the cfe-commits mailing list