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 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
