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