[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()

[Sam Panzer]

I discovered that http://llvm.org/bugs/show_bug.cgi?id=13168 was caused by
incorrectly deciding that variadic functions without FunctionDecl's were
not, in fact, variadic. This has been fixed in the attached patch, which
will look nearly identical as the only change was to move a return
statement out of one enclosing if-else block. I also tested this patch more
extensively, in the hopes of not breaking anything this time :)

On Wed, Jun 20, 2012 at 6:10 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> Thanks, committed as r158887.
>
> On Wed, Jun 20, 2012 at 5:34 PM, Sam Panzer <panzer at google.com> wrote:
> > Hello again,
> >
> > Attached is the newest-and-improved patch.
> >
> > On Wed, Jun 20, 2012 at 4:46 PM, Richard Smith <richard at metafoo.co.uk>
> > wrote:
> >>
> >> Hi Sam,
> >>
> >> On Wed, Jun 20, 2012 at 1:31 PM, Sam Panzer <panzer at google.com> wrote:
> >> > Here is the next version of this patch. Changes over the last one
> >> > include
> >> > Richard's suggestions for variadicArgumentPODCheck, some cleanups that
> >> > were
> >> > available after this adjustment, and more unification of
> >> > Check{Constructor,Function,Block,ObjCMethod}Call(), to make sure that
> >> > the
> >> > warning is issued in a uniform way for various variadic function-like
> >> > things.
> >>
> >> I really like this patch. A handful of quick things, then I think this
> >> is ready to be checked in:
> >>
> >> > +  void checkSecurityProperties(NamedDecl *FDecl, Expr **Args,
> >>
> >> Since this is also checking PODness of vararg types, perhaps checkCall
> >> would be a better name?
> >
> >
> > Good idea; done.
> >
> >>
> >>
> >> > +  StringLiteralCheckType isFormatStringLiteral(const Expr *E,
> >>
> >> Since this actually does non-trivial checking work, perhaps
> >> checkFormatStringExpr?
> >
> >
> > Done.
> >
> >>
> >>
> >> > +  VariadicCallType CallType = //VariadicDoesNotApply;
> >>
> >> Remove this comment.
> >
> >
> > Done.
> >
> >>
> >>
> >> > +    StringLiteralCheckType left =
> >> > +    StringLiteralCheckType right =
> >>
> >> Left, Right.
> >
> >
> > Fixed.
> >
> >>
> >>
> >> > +  if (isValidVarArgType(Ty) != VAK_Valid) {
> >>
> >> This should be == VAK_Invalid, I think.
> >
> >
> > Yep. It was actually correct in my local source...
> >
> >>
> >>
> >> > +  const FunctionProtoType *Proto = NULL;
> >> > +  if ((Proto = dyn_cast<FunctionProtoType>(FuncT))) {
> >>
> >> would be clearer with the dyn_cast in the initialization rather than
> >> assigning in the condition.
> >
> >
> > Done.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120622/5079bbe5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-diag-fixed.patch
Type: application/octet-stream
Size: 52179 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120622/5079bbe5/attachment.obj>