[PATCH] Step 1: Simple Generic lambda (no captures or nested lambdas)
Faisal Vali
faisalv at gmail.com
Mon Jun 17 12:57:30 PDT 2013
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
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