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
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
