[cfe-commits] [PATCH] Sema and AST for C++0x Lambda Expressions
Douglas Gregor
dgregor at apple.com
Fri Aug 26 22:53:35 PDT 2011
On Aug 22, 2011, at 6:44 PM, John Freeman wrote:
> Attached is an updated patch.
>
> - There is a LambdaExpr AST node, and no LambdaDecl.
Okay.
> - Documentation may be sparse in some areas. Please let me know where it is insufficient.
>
> - In the fix-its for explicit captures that conflict with the default, the suggestion is to remove the explicit capture, not the default. My expectation is that this more closely aligns with programmer intent.
Sounds good.
> - I have not yet implemented any traverse/visit/transform functions yet. I want to wait until the AST node is a little more settled before I start that. It seems that some of these functions will have duplicate functionality. Is that true, and is there any opportunity for reuse?
Not much.
> Doug wrote:
>> The ParmVarDecls will be the same in both places. I suggest grabbing them from the Declarator, since that's easiest.
>
> I do not believe it is easier to get ParmVarDecls from a Declarator than from a FunctionProtoTypeLoc. It seems like GetTypeForDeclarator does all the heavy lifting for me. Am I missing something?
Using FunctionProtoTypeLoc to get the ParmVarDecls is fine.
> Doug wrote:
>> The way we typically address this chicken-and-egg problem is to actually build the ParmVarDecls for the constructor before building the constructor itself. The ParmVarDecls are given the translation unit as their DeclContext, and then later (after the constructor is built) we update their DeclContext to point to the constructor. This allows us to build the full type of the constructor before building the constructor itself.
>
> I favored building the type after because it is a little easier. There do not seem to be any problems so far. Should I still build the type first?
+
+/// LambdaExpr - Represents a C++0x lambda expression, e.g.,
+/// @c [&sum] (x) -> void { sum += x; }
+/// Holds related declarations, including the captures and closure type.
This should be (int x) rather than (x), I presume.
+ // FIXME: We can keep this range information separately, or we can go digging
+ // in the ClosureType and Function for the beginning and end every time it is
+ // requested.
+ SourceRange Range;
Typically, we go digging. It isn't all that often that we need the source range, so it isn't worth burning memory on.
+ // FIXME: Do we want to keep the location of the end of the introducer?
Probably not.
+ static LambdaExpr *Create(ASTContext &C, CXXRecordDecl *ClosureType,
+ const Capture *Explicits, unsigned NumExplicits,
+ SourceLocation L) {
+ // FIXME: Set the Expr type later. Is that ok? At this point, the type is
+ // incomplete.
Setting the type later should be fine, although you could probably allocate the CXXRecordDecl and get its type before creating the LambdaExpr. The CXXRecordDecl doesn't have to be complete to form it's type.
+ // FIXME: Should we have this setter?
+ void setExplicits(ASTContext &C, const Capture *E, unsigned N) {
+ size_t Size = N * sizeof(Capture);
+ // Avoid new Capture[] because we don't want to provide a default constructor.
+ void *buffer = C.Allocate(Size, llvm::AlignOf<Capture>::Alignment);
+ memcpy(buffer, E, Size);
+ Explicits = static_cast<Capture*>(buffer);
+ NumExplicits = N;
+ }
I'd prefer to avoid having the setter, if possible. The constructor or Create method can set up the object appropriately.
+ // FIXME: Should we have this setter?
+ //void setClosureType(CXXRecordDecl *D) { ClosureType = D; }
Generally, no, we shouldn't have setters.
--- a/include/clang/AST/RecursiveASTVisitor.h
+++ b/include/clang/AST/RecursiveASTVisitor.h
@@ -1901,6 +1901,10 @@ DEF_TRAVERSE_STMT(CXXConstructExpr, { })
DEF_TRAVERSE_STMT(CallExpr, { })
DEF_TRAVERSE_STMT(CXXMemberCallExpr, { })
+// FIXME: Implement. Is there an order or grouping to these functions?
+// Traverse captures, parameters, return type, body, etc.
+DEF_TRAVERSE_STMT(LambdaExpr, { })
+
Ordering doesn't really matter much here. Ditto for the question in StmtPrinter and StmtProfile.
--- a/lib/Parse/ParseExprCXX.cpp
+++ b/lib/Parse/ParseExprCXX.cpp
@@ -672,8 +672,24 @@ bool Parser::TryParseLambdaIntroducer(LambdaIntroducer &Intro) {
/// expression.
ExprResult Parser::ParseLambdaExpressionAfterIntroducer(
LambdaIntroducer &Intro) {
+ ExprResult MaybeLambda = Actions.ActOnLambdaIntroducer(Intro);
+ if (!MaybeLambda.isUsable())
+ return ExprError();
+
+ // FIXME: get vs. take vs. release vs. takeAs?
take() is fine. The distinction among these is vestigial.
+ // FIXME: I presume we cannot use static_cast since we do not have base class
+ // information for LambdaExpr? Why can't we use llvm::cast? I have a feeling
+ // reinterpret_cast is wrong.
+ LambdaExpr *Lambda = reinterpret_cast<LambdaExpr*>(MaybeLambda.take());
reinterpret_cast is fine.
+ for (llvm::SmallVector<LambdaCapture, 4>::const_iterator
+ C = Intro.Captures.begin(), E = Intro.Captures.end(); C != E; ++C) {
+ if (C->Kind == LCK_This) {
+ if (CapturesThis.isValid()) {
+ Diag(C->Loc, diag::err_capture_more_than_once) << "'this'"
+ << CapturesThis
+ << FixItHint::CreateRemoval(C->Loc);
+ continue;
+ }
Isn't there also a comma to be removed? (Same question for the other Fix-Its removing captures). If the Fix-It isn't going to be perfect (e.g., clean up the code properly, without introducing errors), it shouldn't be there.
+ // FIXME: This accepts register variables. What is the correct test?
+ if (!Var->hasLocalStorage())
+ Diag(C->Loc, diag::err_capture_non_automatic_variable) << C->Id;
You can ask the VarDecl for its storage class.
+ ActOnIdExpression(getCurScope(),
+ ScopeSpec, Name,
+ /*HasTrailingLParen=*/false,
+ /*IsAddressOfOperand=*/false).take());
Rather than using getCurScope(), please pass the Scope* down from the parser.
+ MethodTyInfo = GetTypeForDeclarator(D, getCurScope());
+ // FIXME: Unsure of how to deal with this error. Any problems building the
+ // DeclaratorChunk should have been dealt with earlier, correct? I expect
+ // that we would have a void() function type at worst.
+ // If we can fail here, then probably need to return true to indicate
+ // failure, and have the caller in Parser skip over the rest of the
+ // expression and return ExprError.
+ assert(MethodTyInfo && "no type from lambda-declarator!");
+ MethodTy = MethodTyInfo->getType();
Building the DeclaratorChunks doesn't do all of the semantic checking. For example, GetTypeForDeclarator() could complain if the specified return type of the lambda is an array type, and would return NULL.
+ {
+ // FIXME: Have to use Expr* instead of DeclRefExpr* because we cannot
+ // convert from DeclRefExpr** to Expr**. Why?
Because it would break type safety :)
+ // FIXME: Compare with Sema::BuildMemberInitializer.
+ //*CtorInit = new (Context) CXXCtorInitializer(Context,
+ //Field,
+ //CaptureLoc,
+ ///*L=*/SourceLocation(),
+ //CtorInitArg,
+ ///*R=*/SourceLocation());
Seems like you don't need the FIXME part now?
+ MemInitResult Res = BuildMemberInitializer(Field,
+ &CtorInitArg,
+ /*NumArgs=*/1u,
+ CaptureLoc,
+ /*L=*/SourceLocation(),
+ /*R=*/SourceLocation());
+
+ // FIXME: If there was an error, it is probably a bug, right?
+ assert(Res.isUsable() && "bad member initializer!");
+ *CtorInit = Res.take();
+ ++CtorInit;
Not necessarily a bug! The type might not have a suitable copy constructor, for example, which we would diagnose as an error at this point.
+ {
+ // FIXME: Compare with Sema::BuildCXXConstructExpr.
+ //CXXConstructExpr *CtorCall = CXXConstructExpr::Create(Context,
+ //CtorType,
+ //Lambda->getLocStart(),
+ //Ctor,
+ ///*Elidable=*/false,
+ //CtorArgs.data(),
+ //CtorArgs.size());
The FIXME + commented code isn't needed any more?
+ ExprResult Res = BuildCXXConstructExpr(Lambda->getLocStart(),
+ CtorType,
+ Ctor,
+ /*Elidable=*/false,
+ MultiExprArg(CtorArgs.data(),
+ CtorArgs.size()),
+ /*RequiresZeroInit=*/false,
+ CXXConstructExpr::CK_Complete,
+ /*ParenRange=*/SourceRange());
+
+ // FIXME: If there was an error, it is probably a bug, right?
+ assert(Res.isUsable() && "bad lambda construction!");
In this case, yes, an error here is a compiler bug.
template<typename Derived>
ExprResult
+TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
+ return ExprEmpty();
+}
Please use llvm_unreachable and add a FIXME here. Same for other unimplemented functionality, because we don't want to forget it.
This is looking much better!
- Doug
More information about the cfe-commits
mailing list