[cfe-commits] [PATCH] Parsing C++0x lambda expressions

[Douglas Gregor]

On Jul 6, 2011, at 10:18 AM, John Freeman wrote:

> I'm submitting this patch mainly for review and feedback. It probably isn't ready for commit yet.
> 
> I've included a lot of FIXMEs to document different questions I have and missing pieces. I'll start working on empty Sema actions and lambda-specific Diagnostics while I wait for comments.


A few comments:

Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h	(revision 134493)
+++ include/clang/Parse/Parser.h	(working copy)
@@ -1183,6 +1183,53 @@
                                       bool IsTypename = false);
 
   //===--------------------------------------------------------------------===//
+  // C++0x 5.1.2: Lambda expressions
+
+  enum CXX0XLambdaCaptureDefault {
+    DEFAULT_CAPTURE_NONE,
+    DEFAULT_CAPTURE_BY_COPY,
+    DEFAULT_CAPTURE_BY_REF
+  };
+
+  enum CXX0XLambdaCaptureKind {
+    CAPTURE_BY_COPY,
+    CAPTURE_BY_REF
+  };

Please see

  http://llvm.org/docs/CodingStandards.html#ll_naming

for enumerator naming conventions.

+  // FIXME: This struct is unnecessary at the moment, but if lambda parsing is
+  // divided among several functions, it raises a couple of questions: Should
+  // these be grouped in a struct, or passed as separate parameters? What is the
+  // policy on defining structs just to hold argument packs?
+  struct CXX0XLambdaCaptureList {
+    llvm::SmallVector<CXX0XLambdaCapture, 4> Captures;
+    SourceLocation ThisLoc;
+    CXX0XLambdaCaptureDefault Default;
+
+    CXX0XLambdaCaptureList()
+      : Default(DEFAULT_CAPTURE_NONE) {}
+  };

Why is the location of 'this' kept separately? Shouldn't it just be a CXX0XLambdaCapture, so that it's easier to keep track of the order of the captures?

As for the FIXME, just do whatever feels cleaner to you. There's no specific policy here.

-  case tok::caret:
-    return ParsePostfixExpressionSuffix(ParseBlockLiteralExpression());
-  case tok::code_completion:
+  case tok::caret: {
+    Res = ParseBlockLiteralExpression();
+    break;
+  }

This is a (separable) change.

+/// isCXX0XLambdaExpression - Make sure we are looking at a C++ lambda
+/// expression and not a (possibly nested) Objective-C++ message expression.
+bool Parser::isCXX0XLambdaExpression() {
+  assert(getLang().CPlusPlus0x
+         && Tok.is(tok::l_square)
+         && "why would you call me?");
+  // FIXME: Is there a smaller set of conditions we can check?
+  return !getLang().ObjC1
+         || NextToken().is(tok::r_square)
+         || NextToken().is(tok::equal)
+         || NextToken().is(tok::amp)
+         || (NextToken().is(tok::identifier)
+             && (GetLookAheadToken(2).is(tok::comma)
+                 || GetLookAheadToken(2).is(tok::r_square)));

This isn't quite enough, because of the comma operator:

	[x,y method:17]

To catch this evil case, you're going to need tentative parsing… but only after you've done a quick check for the common/easy cases.

+
+/// ParseCXX0XLambdaExpression - Handle a C++0x lambda expression.
+///
+/// FIXME: Split this into functions for introducer, declarator, and whole
+/// expression?


Only if it becomes unwieldy.

+  bool first = true;
+  while (Tok.isNot(tok::r_square)) {
+    if (first) {
+      first = false;
+
+      // Parse capture-default.
+      if (Tok.is(tok::amp) && NextToken().isNot(tok::identifier)) {
+        List.Default = DEFAULT_CAPTURE_BY_REF;
+      } else if (Tok.is(tok::equal)) {
+        List.Default = DEFAULT_CAPTURE_BY_COPY;
+      }
+
+      if (List.Default != DEFAULT_CAPTURE_NONE) {
+        // Consume '&' or '='.
+        ConsumeToken();
+        continue;
+      }
+    } else {
+      if (ExpectAndConsume(tok::comma,
+                           diag::err_expected_comma,
+                           "",
+                           tok::r_square)) {
+        // FIXME: We could also expect end of capture list. Should the
+        // diagnostic indicate this?
+        return ExprError();
+      }
+    }

I find this logic to be a bit twisty. Why not do an initial check for & (followed by non-identifier) or = to set the default capture kind, then go into the loop that handles individual captures?

+    // Parse capture.
+    CXX0XLambdaCaptureKind Kind = CAPTURE_BY_COPY;
+    if (Tok.is(tok::amp)) {
+      Kind = CAPTURE_BY_REF;
+      ConsumeToken();
+    }
+
+    if (Tok.is(tok::kw_this)) {
+      if (Kind == CAPTURE_BY_REF) {
+        // FIXME: Need proper diagnostic.
+        //fprintf(stderr, "error: 'this' is always captured by copy'");
+      } 
+      if (List.ThisLoc.isValid()) {
+        // FIXME: Need proper diagnostic.
+        //fprintf(stderr,
+                //"error: 'this' may appear only once in a capture list");
+      }
+      List.ThisLoc = Tok.getLocation();
+      ConsumeToken();
+    } else if (Tok.is(tok::identifier)) {
+      List.Captures.push_back(CXX0XLambdaCapture(Tok.getLocation(),
+                                                 Kind,
+                                                 Tok.getIdentifierInfo()));
+      ConsumeToken();
+    } else {
+      // FIXME: Need proper diagnostic.
+      //fprintf(stderr, "error: expected capture in capture list\n");
+      SkipUntil(tok::r_square);
+      return ExprError();
+    }
+  }

Definitely needs some nice diagnostics here :)

+  MatchRHSPunctuation(tok::r_square, IntroLoc);
+

It's probably worth saving the result of MatchRHSPunctuation.

+  // Parse lambda-declarator[opt].
+  // FIXME: Should we default the type quals, or store mutability in this?
+  DeclSpec DS(AttrFactory);

Parse as it's written, and let Sema/AST deal with the mutability.

+  if (Tok.is(tok::l_paren)) {
+    // FIXME: Should we have separate or same scopes for the declarator and
+    // body?
+    ParseScope PrototypeScope(this,
+                              Scope::FunctionPrototypeScope |
+                              Scope::DeclScope);
+

Separate scopes, just like we do with functions.

+    // Parse parameter-declaration-clause.
+    // FIXME: Is "Attr" the correct naming convention for ParsedAttributes?
+    ParsedAttributes Attr(AttrFactory);
+    llvm::SmallVector<DeclaratorChunk::ParamInfo, 16> ParamInfo;
+    SourceLocation EllipsisLoc;


Yes, that's fine.

+    // Parse 'mutable'[opt].
+    if (Tok.is(tok::kw_mutable)) {
+      // FIXME: How should we add this to the declarator? Should we instead
+      // add const-ness if we do not see 'mutable'?
+      ConsumeToken();
+    }

Just extend the Declarator class with a location for the lambda 'mutable' and Sema will deal with it.

+  // Parse compound-statement.
+  if (Tok.is(tok::l_brace)) {
+    // FIXME: Do we need a separate scope type for lambdas, or can we reuse
+    // BlockScope?
+    ParseScope BodyScope(this, Scope::BlockScope | Scope::FnScope |
+                               Scope::BreakScope | Scope::ContinueScope |
+                               Scope::DeclScope);
+
+    StmtResult Stmt(ParseCompoundStatementBody());
+
+    BodyScope.Exit();
+  } else {
+    Diag(Tok, diag::err_expected_fn_body);


Most likely, you can just re-use BlockScope, but it will likely need to be renamed to ClosureScope.

	- Doug