Hi Chandler,
All points noted, new patch incoming by the end of the day. Cheers, James From: Chandler Carruth [mailto:[email protected]] Sent: 26 January 2012 11:17 To: James Molloy Cc: [email protected]; [email protected]; [email protected] Subject: Re: [cfe-commits] [PATCH] Fix tag decls/enum constants in function prototypes On Thu, Jan 26, 2012 at 3:01 AM, James Molloy <[email protected]> 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
