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

[Chandler Carruth]

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120126/10cdbaab/attachment.html>