[PATCH] Step 1: Simple Generic lambda (no captures or nested lambdas)
Richard Smith
richard at metafoo.co.uk
Sun Jun 16 16:48:11 PDT 2013
On Sun, Jun 16, 2013 at 7:24 AM, Faisal Vali <faisalv at gmail.com> wrote:
> 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).
> - 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?
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