On Mon, Jun 17, 2013 at 2:57 PM, Faisal Vali <[email protected]> wrote: > On Mon, Jun 17, 2013 at 2:38 PM, Richard Smith <[email protected]> wrote: >> On Mon, Jun 17, 2013 at 11:50 AM, Faisal Vali <[email protected]> wrote: >>> 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? >> >> It seems to be carrying information (specifically, the >> capture-default) that can't be recovered in any other way during the >> instantiation of the lambda call operator, so this seems OK. I suspect >> you won't need this any more once captures are fully implemented, >> since instantiating the call operator shouldn't trigger any new >> captures (instead, all captures should have been discovered when >> instantiating the lambda expression itself). > > You're probably right - but it might be helpful to know the capture > default during emission > of a diagnostic for a certain instantiation of the generic lambda (but > I could be wrong, > so lets see once captures get fully implemented). > > Also if it's ok to leave the LambdaExpr in there, i can at least query > some of the operators > straight off of there. I would have thought it would be less
This is incorrect. The LambdaExpr forwards its calls to the class. > efficient to look operators up than > to cache pointers to them (which is why i added them to CXXRecordDecl) > - but i guess the > spatial concerns win out here (because they would have a more pervasive > effect)? > > Thanks! > > >> >>> 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
