[cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes

[James Molloy]

Hi,

 

Ping^2. Chandler, if you don’t have time to complete this review could someone else please do it? It’s been sitting around for quite a while now.

 

Cheers,

 

James

 

From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of James Molloy
Sent: 01 February 2012 10:03
To: 'Chandler Carruth'
Cc: juli at clockworksquid.com; cfe-commits at cs.uiuc.edu; ken.dyck at onsemi.com
Subject: Re: [cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes

 

Hi Chandler,

 

The simple answer is that at the point of parsing the function parameter type (which these decls are), the current context is just the translation unit. There is no prototype context entered.

 

Even function parameters (ParmDecls) get pinned to the translation unit scope – CurContext.isTranslationUnit() is true when Sema is about to create a function parameter and there’s even this comment which makes that state tautologous:

 

  // Temporarily put parameter variables in the translation unit, not

  // the enclosing context.  This prevents them from accidentally

  // looking like class members in C++.

  ParmVarDecl *New = CheckParameter(Context.getTranslationUnitDecl(),

 

So basically, Clang doesn’t have a good mechanism for this and my patch just uses the same mechanism as is already there. I’m not proficient enough with Clang to rewrite part of the AST structure to make both function parameters and decls in parameters less hacky.

 

Cheers,

 

James

 

From: Chandler Carruth [mailto:chandlerc at google.com] 
Sent: 01 February 2012 08:43
To: James Molloy
Cc: juli at clockworksquid.com; cfe-commits at cs.uiuc.edu; ken.dyck at onsemi.com
Subject: Re: [cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes

 

Haven't yet done a full review, sorry for the delays, but wanted to ask one question that jumped out at me. Maybe you can explain and save me the trouble of tracking it down.

On Tue, Jan 31, 2012 at 1:41 AM, James Molloy <james.molloy at arm.com> wrote:

This is due to a hack in the parser that hangs such tags on the translation unit because no proper declcontext is available at that point. I've modified my code so it catches this and unhooks them from the translation unit, so they don't get visited twice and the output is better.

 

It would seem more clean to go in and fix the code that attaches these to the translation unit to not do so? The current pattern seems very inconsistent. It also means we're mutating the AST in weird ways. That makes me nervous. I'd essentially like to see far fewer conditionals when handling these declarations. We should be able to unconditionally set up the lexical context, attach to the function, and push onto the scope... If there are conditions on these steps, I would benefit from comments documenting why we don't have consistent behavior...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120208/57c6cee4/attachment.html>