On Sun, Jun 16, 2013 at 6:48 PM, Richard Smith <[email protected]> wrote: > On Sun, Jun 16, 2013 at 7:24 AM, Faisal Vali <[email protected]> wrote: <snip> > >> - addressed the recursive instantiation bug (which only seemed to manifest >> during diagnostic emission) with the much cleaner fix suggested by richard >> (swapping the localpendinginstantiations in and out) >> >> >> All clang tests pass (except for the ones having to do with Index and Modules >> that failed for me prior to the changes) >> >> Can someone ;) please give me a sense of how far this is from being >> commit-ready? > > I don't think it's far off. Some comments on the patch: > > It seems surprising to me that you need to save the CallOperator and > StaticInvoker on the LambdaDefinitionData class; could you find those > when needed by looking them up within the RecordDecl?
Yes, I'll get rid of those. But can I leave the LambdaExpr* member in LambdaDefinitionData? In regards to the rest of your comments below - thank you - i'll respond once i've had time to look into them further. >> This updated patch implements simple non-capturing, non-nested generic >> lambdas, with return type deduction. The changes from the previous patch: >> >> - added basic serialization/deserialization code (all pch tests pass, i added >> some simple generic lambda ones) >> - added support for serializing/deserializing the CXXRecordDecl >> LambdaDataDefinition members I added >> - also added ser/deser support for PendingLocalImplicitInstantiations >> (else the uninstantiated generic lambdas create inconsistencies with >> pch) > > This implies there is still a bug somewhere. Pending local > instantiations must be instantiated in some local context; if any are > left when we get to the end of a PCH, we've done something wrong. I > think you just need to swap the instantiations back earlier > (immediately after the call to PerformPendingInstantiations). > > I don't think you need the IsParameterPack parameter to AutoType -- > instead, you could look at whether the > DeducedType.containsUnexpandedParameterPack(). > > IsGenericLambdaAutoParameter in Sema::Declarator should be a bitfield, > but I think this would be better handled by instead adding a new kind > of declarator context for lambda parameters. > > I would prefer for the parser to not need to know about the > implicitly-generated template parameters, if you can arrange that. > Maybe the relevant data can be maintained by Sema in the > LambdaScopeInfo? > > Are the changes to ActOnReturnStmt necessary? ActOnCapScopeReturnStmt > already deals with auto return types for lambdas, and has some other > special cases which should be applied to lambdas (different > diagnostics, slightly different rules). > > The patch has some tabs in it, and other unexpected whitespace changes. ---------------------------- > >> On Sat, Jun 15, 2013 at 3:04 PM, Richard Smith <[email protected]> wrote: >>> On Sat, Jun 15, 2013 at 7:34 AM, Faisal Vali <[email protected]> wrote: >>>> >>>> This updated patch implements simple non-capturing, non-nested generic >>>> lambdas: >>>> >>>> As an example of what compiles: >>>> >>>> template <class F1, class F2> >>>> struct overload : F1, F2 { >>>> using F1::operator(); >>>> using F2::operator(); >>>> overload(F1 f1, F2 f2) : F1(f1), F2(f2) { } >>>> }; >>>> >>>> auto Recursive = [](auto Self, auto h, auto ... rest) { >>>> return 1 + Self(Self, rest...); >>>> }; >>>> auto Base = [](auto Self, auto h) { >>>> return 1; >>>> }; >>>> overload<decltype(Base), decltype(Recursive)> O(Base, Recursive); >>>> int num_params = O(O, 5, 3, "abc", 3.14, 'a'); >>>> >>>> Please see attached tests for more examples. >>>> >>>> >>>> Changes from the initial version of this patch: >>>> - added return type deduction (since the changes were minimal) >>>> -- both C++11 and C++1y deduction is supported for lambdas >>>> -- in c++1y mode we replace the implicit trailing return type with >>>> 'auto' >>>> instead of 'dependent type' (c++11) >>>> -- in c++1y mode forward to Richard's deduction machinery, else >>>> to Doug's original deduction machinery (there are some minor >>>> differences in deduction, hopefully subtly captured in the tests) >>>> - Per feedback from Eli: >>>> -- the ActOnAutoParameter has been buried within ActOnParamDeclarator >>>> -- ConversionOperator property has been removed >>>> -- "__invoke" is being used in the one place it is needed in the >>>> static invoker >>>> >>>> Issues: >>>> - the pch test file cxx11-lambda.mm causes a crash, and I do not know >>>> enough about PCH to try and tackle it without some guidance. >>>> - I'm still hoping Doug or Richard or another Clang template >>>> instantiation >>>> wizard can comment on the recursive instantiation issue. >>> >>> >>> I think you just need to save and clear PendingLocalImplicitInstantiations >>> before you start instantiating a function definition, and restore it >>> afterwards (just like we already do for PendingInstantiations, but not >>> conditional on the 'Recursive' flag). >>> >>>> >>>> Next TODO: >>>> - once i get this patch approved and committed, i will start >>>> merging/working on >>>> capturing capability and nested lambdas. >>>> >>>> Look forward to your timely :) review and feedback! >>>> >>>> Thanks! >>>> >>>> >>>> Faisal Vali >>>> >>>> >>>> >>>> On Fri, Jun 14, 2013 at 12:26 PM, Eli Friedman <[email protected]> >>>> wrote: >>>> > On Fri, Jun 14, 2013 at 10:01 AM, Faisal Vali <[email protected]> wrote: >>>> >> >>>> >> On Thu, Jun 13, 2013 at 12:58 PM, Eli Friedman <[email protected]> >>>> >> wrote: >>>> >> > On Tue, Jun 11, 2013 at 10:05 PM, Faisal Vali <[email protected]> >>>> >> > wrote: >>>> >> > @@ -547,6 +550,18 @@ >>>> >> > >>>> >> > /// \brief The type of the call method. >>>> >> > TypeSourceInfo *MethodTyInfo; >>>> >> > + >>>> >> > + /// \brief The Lambda call method >>>> >> > + CXXMethodDecl *CallOperator; >>>> >> > + >>>> >> > + /// \brief The Lambda conversion operator, for non-capturing >>>> >> > lambdas >>>> >> > + CXXConversionDecl *ConversionOperator; >>>> >> > + >>>> >> > + /// \brief The Lambda static method invoker, for non-capturing >>>> >> > lambdas >>>> >> > + CXXMethodDecl *StaticInvoker; >>>> >> > + >>>> >> > + LambdaExpr *ParentLambdaExpr; >>>> >> > >>>> >> > It's not obvious why you're making this change... we try to keep the >>>> >> > AST >>>> >> > as >>>> >> > small as possible. >>>> >> > >>>> >> >>>> >> I see, but how do you suggest I access this data when I need it? >>>> > >>>> > >>>> > If you really do need it all, it's okay, I guess... it was just a bit >>>> > surprising at first glance. Taking another look, it doesn't look like >>>> > you're actually using getLambdaConversionOperator() (and in clang's >>>> > implementation, lambdas can have multiple conversion operators). >>>> > >>>> >> >>>> >> >>>> >> > >>>> >> > + ParmVarDecl *PVD = cast<ParmVarDecl>(Param); >>>> >> > + // Handle 'auto' within a generic lambda. >>>> >> > + // FVQUESTION: Should I use getOriginalType here - how do i >>>> >> > + // know when to use which? >>>> >> > + QualType ParamType = PVD->getType(); >>>> >> > >>>> >> > If you're unsure, getType() is probably the right choice... I don't >>>> >> > think it >>>> >> > matters here, though. >>>> >> > >>>> >> >>>> >> Thanks! What is the difference between the two? >>>> > >>>> > >>>> > getOriginalType() returns the undecayed type; getType() returns the >>>> > semantic >>>> > type of the declaration. >>>> > >>>> >> >>>> >> >>>> >> > + if (getLangOpts().CPlusPlus1y && >>>> >> > ParamType->getContainedAutoType()) { >>>> >> > + TemplateTypeParmDecl *TemplateParam = >>>> >> > + Actions.ActOnLambdaAutoParameter(getCurScope(), PVD, >>>> >> > + CurTemplateDepth, CurAutoParameterIndex); >>>> >> > + TemplateParams.push_back(TemplateParam); >>>> >> > + } >>>> >> > >>>> >> > We generally prefer to avoid having code in Parser/ inspect AST >>>> >> > types. >>>> >> > Can >>>> >> > you move building the TemplateParameterList into >>>> >> > ActOnStartOfLambdaDefinition? >>>> >> > >>>> >> >>>> >> >>>> >> In my initial implementation of generic lambdas, I had encased this >>>> >> transformation >>>> >> entirely within ActOnStartOfLambdaDefinition (which is called once the >>>> >> parameter >>>> >> declaration clause has been constructed). In that version, I >>>> >> reconstructed >>>> >> the typesourceinfo and function type of the call operator after it was >>>> >> initially >>>> >> parsed and semanalyzed, by replacing each auto with a template type >>>> >> paramater, >>>> >> creating the template prameter list, and using a visitor to fix all >>>> >> subsequent references (ugh!) >>>> >> >>>> >> Based on sage advice from Doug, I moved the transformation upstream >>>> >> and the code changes >>>> >> have proved to be much leaner. >>>> > >>>> > >>>> > Okay. >>>> > >>>> >> In regards to avoiding having the Parser inspect AST types What I >>>> >> could do is nest >>>> >> ActOnAutoParameter within ActOnParamDeclarator. Is it ok that I pass >>>> >> in a vector >>>> >> of TemplateTypeParamDecl* as an out parameter to >>>> >> ParseParameterDeclarationClause >>>> >> which I will then need to pass in to ActOnParamDeclarator? Or is it >>>> >> important that I find a way to do it without passing that vector >>>> >> around? >>>> > >>>> > >>>> > That should be fine. >>>> > >>>> >> >>>> >> > +// The name of the static invoker function that will forward >>>> >> > +// to the corresponding lambda call operator. >>>> >> > +static inline const char* getSecretLambdaStaticInvokerStringID() { >>>> >> > + return "__invoker"; >>>> >> > +} >>>> >> > >>>> >> > I'm not sure how this abstraction is actually helpful, given the >>>> >> > function >>>> >> > only has one use. >>>> >> > >>>> >> >>>> >> I figured it might be less brittle if more uses cropped up in the >>>> >> future. >>>> >> But >>>> >> I can just insert the name: "__invoker" in the one place i need it, and >>>> >> drop >>>> >> this function? >>>> > >>>> > >>>> > I think so. We can always change it when we add more uses. >>>> > >>>> >> >>>> >> > + // FVQUESTION? When a generic lamdba call operator is being >>>> >> > instantiated, >>>> >> > if >>>> >> > + // for some reason instantiation fails, the SemaDiagnosticEmitter >>>> >> > ends up >>>> >> > + // emitting the entire instantiation stack, and since local >>>> >> > pending >>>> >> > + // instantiations are called recursively, >>>> >> > >>>> >> > We should not be instantiating the lambda in your example more than >>>> >> > once >>>> >> > at >>>> >> > the same time. If we are, it's probably a bug. >>>> >> > >>>> >> >>>> >> >>>> >> Does my workaround seem reasonable? >>>> >> >>>> > >>>> > Sorry, I'm not really an expert on template instantiation. Hopefully >>>> > Doug >>>> > will chime in. >>>> > >>>> > -Eli >>>> > >>> >>> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
