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