[PATCH] Instantiate static constexpr member function of a local struct in a function template earlier.

[Richard Smith]

Your analysis makes sense to me. I've committed your updated patch in
r237064. Thank you!

On Thu, May 7, 2015 at 12:53 AM, Michael Park <mcypark at gmail.com> wrote:

> ping.
>
> On 30 April 2015 at 22:46, Michael Park <mcypark at gmail.com> wrote:
>
>> I've got a follow-up patch which I think addresses an existing bug that
>> hadn't surfaced.
>>
>> In the *ParseLateTemplatedFuncDef* function in
>> *lib/Parse/ParseTemplate.cpp*, we rebuild the *DeclContext* starting
>> from the function decl context up to the translation unit context with the
>> following logic:
>>
>>   // Get the list of DeclContexts to reenter.
>>>   SmallVector<DeclContext*, 4> DeclContextsToReenter;
>>>   DeclContext *DD = FunD;
>>>   while (DD && *!DD->isTranslationUnit()*) {
>>>     DeclContextsToReenter.push_back(DD);
>>>     DD = DD->getLexicalParent();
>>>   }
>>
>>
>> Then we reenter the sequence of decl contexts in reverse, with
>> *PushDeclContext*.
>>
>>   // Reenter template scopes from outermost to innermost.
>>>   SmallVectorImpl<DeclContext *>::reverse_iterator II =
>>>       DeclContextsToReenter.rbegin();
>>>   for (; II != DeclContextsToReenter.rend(); ++II) {
>>>     TemplateParamScopeStack.push_back(new ParseScope(this,
>>>           Scope::TemplateParamScope));
>>>     unsigned NumParamLists =
>>>       Actions.ActOnReenterTemplateScope(getCurScope(), cast<Decl>(*II));
>>>     CurTemplateDepthTracker.addDepth(NumParamLists);
>>>     if (*II != FunD) {
>>>       TemplateParamScopeStack.push_back(new ParseScope(this,
>>> Scope::DeclScope));
>>>
>>> *Actions.PushDeclContext(Actions.getCurScope(), *II);*    }
>>>   }
>>
>>
>> This means that *Actions*'* CurContext* must be the translation unit
>> context, otherwise the assertion in *PushDeclContext* fails. I think the
>> intention of the following *ContextRAII* was to satisfy this
>> pre-condition, but instead we save the *CurContext* and stay in the
>> *CurContext*.
>>
>> // To restore the context after late parsing.
>>> Sema::ContextRAII GlobalSavedContext(Actions, Actions.CurContext);
>>
>>
>> The fix is to save the *CurContext* then jump to the translation unit
>> context in order to satisfy the pre-conditions necessary to rebuild and
>> reenter the decl contexts.
>>
>> // To restore the context after late parsing.
>>> Sema::ContextRAII GlobalSavedContext(
>>
>>     Actions, Actions.Context.getTranslationUnitDecl());
>>
>>
>> On 1 May 2015 at 01:25, Michael Park <mcypark at gmail.com> wrote:
>>
>>> I've got a follow-up patch which I think addresses an existing bug that
>>> hadn't surfaced.
>>>
>>> In the *ParseLateTemplatedFuncDef* function in
>>> *lib/Parse/ParseTemplate.cpp*, we rebuild the *DeclContext* starting
>>> from the function decl context up to the translation unit context with the
>>> following logic:
>>>
>>>   // Get the list of DeclContexts to reenter.
>>>>   SmallVector<DeclContext*, 4> DeclContextsToReenter;
>>>>   DeclContext *DD = FunD;
>>>>   while (DD && *!DD->isTranslationUnit()*) {
>>>>     DeclContextsToReenter.push_back(DD);
>>>>     DD = DD->getLexicalParent();
>>>>   }
>>>
>>>
>>> Then we reenter the sequence of decl contexts in reverse, with
>>> *PushDeclContext*.
>>>
>>>   // Reenter template scopes from outermost to innermost.
>>>>   SmallVectorImpl<DeclContext *>::reverse_iterator II =
>>>>       DeclContextsToReenter.rbegin();
>>>>   for (; II != DeclContextsToReenter.rend(); ++II) {
>>>>     TemplateParamScopeStack.push_back(new ParseScope(this,
>>>>           Scope::TemplateParamScope));
>>>>     unsigned NumParamLists =
>>>>       Actions.ActOnReenterTemplateScope(getCurScope(), cast<Decl>(*II));
>>>>     CurTemplateDepthTracker.addDepth(NumParamLists);
>>>>     if (*II != FunD) {
>>>>       TemplateParamScopeStack.push_back(new ParseScope(this,
>>>> Scope::DeclScope));
>>>>
>>>> *Actions.PushDeclContext(Actions.getCurScope(), *II);*    }
>>>>   }
>>>
>>>
>>> This means that *Actions*'* CurContext* must be the translation unit
>>> context, otherwise the assertion in *PushDeclContext* fails. I think
>>> the intention of the following *ContextRAII* was to satisfy this
>>> pre-condition, but instead we save the *CurContext* and stay in the
>>> *CurContext*.
>>>
>>> // To restore the context after late parsing.
>>>> Sema::ContextRAII GlobalSavedContext(Actions, Actions.CurContext);
>>>
>>>
>>> The fix is to save the *CurContext* then jump to the translation unit
>>> context in order to satisfy the pre-conditions necessary to rebuild and
>>> reenter the decl contexts.
>>>
>>> // To restore the context after late parsing.
>>>> Sema::ContextRAII GlobalSavedContext(
>>>
>>>     Actions, Actions.Context.getTranslationUnitDecl());
>>>
>>>
>>> On 1 May 2015 at 01:12, Michael Park <mcypark at gmail.com> wrote:
>>>
>>>> I've got a follow-up patch which I think addresses an existing bug that
>>>> hadn't surfaced.
>>>>
>>>> In the *ParseLateTemplatedFuncDef* function in
>>>> *lib/Parse/ParseTemplate.cpp*, we build up the sequence of DeclContext
>>>>
>>>>
>>>> On 29 April 2015 at 11:20, Michael Park <mcypark at gmail.com> wrote:
>>>>
>>>>> Ah, that's unfortunate. I'll take a look. Thanks!
>>>>>
>>>>> On 28 April 2015 at 20:33, Richard Smith <richard at metafoo.co.uk>
>>>>> wrote:
>>>>>
>>>>>> One of our buildbots seems unhappy:
>>>>>>
>>>>>>
>>>>>> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/11939/steps/test_all/logs/Clang-Unit%20%3A%3A%20ASTMatchers__ASTMatchersTests__HasAncestor.MatchesClosestAncestor
>>>>>>
>>>>>> Its testcase is this:
>>>>>>
>>>>>>       template <typename T> struct C {
>>>>>>         void f(int) {
>>>>>>           struct I { void g(T) { int x; } } i; i.g(42);
>>>>>>         }
>>>>>>       };
>>>>>>       template struct C<int>;
>>>>>>
>>>>>> It looks like the problem is some interaction of your patch and
>>>>>> delayed template parsing (running the above code through clang with
>>>>>> -fdelayed-template-parsing reproduces the issue). Can you take a look?
>>>>>>
>>>>>> On Tue, Apr 28, 2015 at 5:23 PM, Michael Park <mcypark at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Awesome, Thanks Richard!
>>>>>>>
>>>>>>> On 28 April 2015 at 20:11, Richard Smith <richard at metafoo.co.uk>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> On Tue, Apr 28, 2015 at 4:00 PM, Michael Park <mcypark at gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> The answer is that we don't really know. It would be more
>>>>>>>>>> consistent with the non-template case for it to be ill-formed (it also
>>>>>>>>>> seems reasonable to say that you can't instantiate a member function until
>>>>>>>>>> its class is complete), but the point of instantiation rules for constexpr
>>>>>>>>>> functions aren't settled yet; this is part of DR1581 (still open).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Agreed. Thanks for the pointer to DR1581!
>>>>>>>>>
>>>>>>>>> LGTM
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> As this is my first patch for Clang, I'm not all that familiar
>>>>>>>>> with the required process to get this patch committed. Am I required to do
>>>>>>>>> anything further on my end?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I've committed your patch as r236063. Feel free to go ahead and
>>>>>>>> mark PR20625 as fixed. Thanks!
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> MPark.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150511/181f2b69/attachment.html>