<div style="font-family: arial, helvetica, sans-serif"><font size="2">Hello again,<div><br></div><div>Attached is the newest-and-improved patch.</div><div><br><div class="gmail_quote">On Wed, Jun 20, 2012 at 4:46 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sam,<br>
<div class="im"><br>
On Wed, Jun 20, 2012 at 1:31 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
> Here is the next version of this patch. Changes over the last one include<br>
> Richard's suggestions for variadicArgumentPODCheck, some cleanups that were<br>
> available after this adjustment, and more unification of<br>
> Check{Constructor,Function,Block,ObjCMethod}Call(), to make sure that the<br>
> warning is issued in a uniform way for various variadic function-like<br>
> things.<br>
<br>
</div>I really like this patch. A handful of quick things, then I think this<br>
is ready to be checked in:<br>
<br>
> +  void checkSecurityProperties(NamedDecl *FDecl, Expr **Args,<br>
<br>
Since this is also checking PODness of vararg types, perhaps checkCall<br>
would be a better name?<br></blockquote><div><br></div><div>Good idea; done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  StringLiteralCheckType isFormatStringLiteral(const Expr *E,<br>
<br>
Since this actually does non-trivial checking work, perhaps<br>
checkFormatStringExpr?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  VariadicCallType CallType = //VariadicDoesNotApply;<br>
<br>
Remove this comment.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +    StringLiteralCheckType left =<br>
> +    StringLiteralCheckType right =<br>
<br>
Left, Right.<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +  if (isValidVarArgType(Ty) != VAK_Valid) {<br>
<br>
This should be == VAK_Invalid, I think.<br></blockquote><div><br></div><div>Yep. It was actually correct in my local source...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
> +  const FunctionProtoType *Proto = NULL;<br>
> +  if ((Proto = dyn_cast<FunctionProtoType>(FuncT))) {<br>
<br>
would be clearer with the dyn_cast in the initialization rather than<br>
assigning in the condition.<br></blockquote><div><br></div><div>Done. </div></div></div></font></div>