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

[James Molloy]

Hi Chandler,

> Interesting... Maybe we're already visiting them somewhere else, perhaps in the outer decl context? It'd be good to verify what's going on here. It's unclear to me whether this requires no change, strictly an additive change (visit these things), or moving the visit that exists to occur at the right point in the visitation sequence....

It was unclear to me too, so I have investigated it.

All of the visitors/dumpers were already visiting these tags. This is because the issue is purely to do with scoping - the tag was still attached to the ParmVarDecl as its Type, so was already being visited.

For example: "void f(enum {X} p)" - the ast printer prints "void f(<anonymous enum at...> p)". It would also however print the contents of the enum at the translation unit scope. 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.

I have also modified the XML dumper to print these tags, which it wasn't currently doing (aside: is the XML dumper deprecated?)

I think I've covered all your other points - I'm going to add a warning group -Wvisibility, which will cover the two warnings mentioned (warn_decl_in_param_list and the new warn_redefinition_in_param_list).

Hopefully this should be OK now?

Cheers,

James

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

On Thu, Jan 26, 2012 at 3:01 AM, James Molloy <james.molloy at arm.com> wrote:
No problem. This is a convention in LLVM’s test dirs

Just as an FYI, I think the convention is going away even in LLVM. Many new tests getting added don't have a date, and I'm hoping to see a continuing trend away from them.

Separation of concerns is good. Yes, I’m adding a new warning that has no flag, but it’s essentially a clone of another warning with one verb different that currently has no flag. Do you suggest I add the flag to cover both the old and new warnings in the same patch? I can do it as a followup and I’d prefer that if possible.

This is a tricky point. The thing is that this file is specifically designed to prevent the slow creep for the reasons you describe. The logic you use got us into a bad state where most warnings don't have flags.

That said, I support separate commits, I would just invert the order. Add the flag to the existing warning, and remove it from the file, then proceed with this patch, using the newly available flag. That way the test policy is maintained and it never shrinks. It's a bit of a refactoring tax, but probably a healthy one considering how bad this got.
 
 - You need to add visiting logic for all of these decls to the various visitors I suspect. We have several, I'd have to go looking to enumerate all of them. Off the top of my head I would check:
  - RecursiveASTVisitor descends into these decls.
  - The various printing and dumping visitors catch them
  - The CFG builder handles them (not sure how much it really has to care)
  - Maybe update libclang? Not likely important for the first pass.
 
Thanks for catching this – I didn’t think it would be required. I’ll ensure everything acts as it should, although I’m not sure how much effect this will have. The decls already existed previously, just were in the wrong scope.

Interesting... Maybe we're already visiting them somewhere else, perhaps in the outer decl context? It'd be good to verify what's going on here. It's unclear to me whether this requires no change, strictly an additive change (visit these things), or moving the visit that exists to occur at the right point in the visitation sequence....

Looking at the output of AST dump with some obvious test cases will likely make it obvious what the right thing is to do here.
 
It’s raw new’d for the reason you state. I didn’t use arrayref because the existing code for ParmVarDecl (see diff context)  also doesn’t use ArrayRef. I copied its interface completely, assuming (for better or worse) that it was designed that way for a reason.

Hmm, I fear this may not be a good assumption. ;] Technically, ArrayRef isn't as good of a fit for the Parm ones because we have external knowledge about how many of those there are. Using an arrayref would waste at least 4 bytes of storage. Since you need both the pointer and the size, might as well use ArrayRef.

Here and below where the same issue arose, I would encourage you to use the simplest and most minimal interface that works for your use case. If the surrounding code has a more complex pattern that isn't actually needed, let's simplify that as well to get consistency. (Naturally, I would do that in a separate patch, it can even be another 'pre factoring' patch if you will). I don't want us to become complacent in the pursuit of consistency and miss opportunities to use better and/or simpler interfaces.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enum_patch_v2.diff
Type: application/octet-stream
Size: 16020 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120127/17710ecb/attachment.obj>