[PATCH] Step 1: Simple Generic lambda (no captures or nested lambdas)
Faisal Vali
faisalv at gmail.com
Tue Jun 18 14:27:07 PDT 2013
hi Richard, in regards to:
>>>>> - 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).
>>>
Currently in clang, the following code leaves an instantiation
in the PendingLocalInstantiation queue, which gets instantiated
at the endof the translation unit (if not in a pch context).
int foo(double y) {
struct Lambda {
template<class T> T operator()(T t) const { return t; };
} lambda;
return lambda(y);
}
In a pch context, this does not get instantiated at the end of the
translation unit
(see ActOnEndOfTU) - regardless of where i swap the pending instantiations.
If you're saying that this is a bug, then I wonder if ActOnFinishFunctionBody
should call PerformPendingInstantiations(/*LocalOnly*/true) to ensure that
all local instantiations should be instantiated while that function's context
is still on the stack? If that is the case, do i need to maintain a
stack of local pending
instantiations from which i swap in and out of, so that deeply nested member
functions only instantiate those instantiations that they added to the
pending list?
Let me know your thoughts.
thanks!
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
> 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
>>>
>>>>
>>>> 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