[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[Jordan Rose]

Sorry to have let this slip! This is looking good, but I had one more thought about the diagnostic message. It says "may yield unexpected results", but doesn't really explain what those unexpected results are. I was wondering if we could work the type into the message for the operator case.

"operand of sizeof is a binary expression of type %0, which may not be intended"

I don't like that wording either, but at least this one makes people say "what? why isn't it [the type I actually want]?". Also, should there be a way to silence the warning?

What do you think?
Jordan


On Jan 23, 2014, at 6:40 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

> Hi,
> 
> New one with comments handled.
> 
> ________________________________________
> Från: Jordan Rose [jordan_rose at apple.com]
> Skickat: den 20 december 2013 19:15
> Till: Anders Rönnholm
> Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki; Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> 
> On Dec 10, 2013, at 4:38 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
> 
> Are you OK to commit this patch or do you see more issues?
> 
> I'm not sure if anyone else has ideological concerns. There's always a flag to turn this off, I suppose.
> 
> 
> +  if (S.isSFINAEContext())
> +      return;
> 
> Code style: extra indent?
> 
> 
> +  if(E->HasSideEffects(S.getASTContext()))
> +    return;
> 
> sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't want to warn here.
> 
> 
> +  const FunctionDecl *FD = S.getCurFunctionDecl();
> +  if(FD && FD->isFunctionTemplateSpecialization())
> +    return;
> 
> Code style: space after if. (Above too, actually.)
> 
> 
> +    if (Binop->getLHS()->getType()->isArrayType() ||
> +        Binop->getLHS()->getType()->isAnyPointerType() ||
> +        Binop->getRHS()->getType()->isArrayType() ||
> +        Binop->getRHS()->getType()->isAnyPointerType())
> +      return;
> 
> I don't think this is correct...the user is only trying to get ptrdiff_t if both the LHS and RHS are pointer-ish.
> 
> 
> +def warn_sizeof_bin_op : Warning<
> +  "using sizeof() on an expression with an operator may yield unexpected results">,
> +  InGroup<SizeofOnExpression>;
> +
> +def warn_sizeof_sizeof : Warning<
> +  "using sizeof() on sizeof() may yield unexpected results.">,
> +  InGroup<SizeofOnExpression>;
> +
> 
> sizeof doesn't actually require parens, so we shouldn't put the parens in the diagnostics.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140131/04e87270/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeofonexpression.diff
Type: application/octet-stream
Size: 7573 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140131/04e87270/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140131/04e87270/attachment-0001.html>