[PATCH] Step 1: Simple Generic lambda (no captures or nested lambdas)
Faisal Vali
faisalv at gmail.com
Mon Jun 17 14:27:42 PDT 2013
On Mon, Jun 17, 2013 at 2:57 PM, Faisal Vali <faisalv at gmail.com> wrote:
> On Mon, Jun 17, 2013 at 2:38 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Mon, Jun 17, 2013 at 11:50 AM, Faisal Vali <faisalv at gmail.com> wrote:
>>> On Sun, Jun 16, 2013 at 6:48 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>> On Sun, Jun 16, 2013 at 7:24 AM, Faisal Vali <faisalv at gmail.com> 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 <richard at metafoo.co.uk> wrote:
>>>>>> On Sat, Jun 15, 2013 at 7:34 AM, Faisal Vali <faisalv at gmail.com> 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 <eli.friedman at gmail.com>
>>>>>>> wrote:
>>>>>>> > On Fri, Jun 14, 2013 at 10:01 AM, Faisal Vali <faisalv at gmail.com> wrote:
>>>>>>> >>
>>>>>>> >> On Thu, Jun 13, 2013 at 12:58 PM, Eli Friedman <eli.friedman at gmail.com>
>>>>>>> >> wrote:
>>>>>>> >> > On Tue, Jun 11, 2013 at 10:05 PM, Faisal Vali <faisalv at gmail.com>
>>>>>>> >> > 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
>>>>>>> >
>>>>>>
>>>>>>
More information about the cfe-commits
mailing list