<div class="gmail_quote">On Thu, Jan 26, 2012 at 3:01 AM, James Molloy <span dir="ltr"><<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="color:rgb(31,73,125);font-family:Calibri,sans-serif;font-size:11pt">No problem. This is a convention in LLVM’s test dirs</span></p></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">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.</span></p></div></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> </span>- 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:</p></div><div class="im"><div><p class="MsoNormal"><u></u></p>
</div>
<div>
<p class="MsoNormal"> - RecursiveASTVisitor descends into these decls.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> - The various printing and dumping visitors catch
them<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> - The CFG builder handles them (not sure how much it
really has to care)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> - Maybe update libclang? Not likely important for the
first pass.<u></u><u></u></p>
</div>
</div><div>
<p class="MsoNormal"><span style="color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">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.</span></p></div></div></div></blockquote><div><br></div><div>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....</div>
<div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple"><div><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
</div><div class="im">
<div>
<p class="MsoNormal"><span style="color:rgb(31,73,125);font-family:Calibri,sans-serif;font-size:11pt">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.</span></p></div></div></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
</div>