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 <[email protected]> wrote: > ping. > > On 30 April 2015 at 22:46, Michael Park <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >>>> >>>>> Ah, that's unfortunate. I'll take a look. Thanks! >>>>> >>>>> On 28 April 2015 at 20:33, Richard Smith <[email protected]> >>>>> 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 <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Awesome, Thanks Richard! >>>>>>> >>>>>>> On 28 April 2015 at 20:11, Richard Smith <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> On Tue, Apr 28, 2015 at 4:00 PM, Michael Park <[email protected]> >>>>>>>> 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. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
