Sorry for letting this slip through the cracks! I know it's now been a month
and a half, but what were the false positives you saw without the
HasSideEffects check? For example:
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1);
+} // no-warning
This should have a warning, since the function is not called. If it interferes
with the VLA thing Aaron brought up, though...
I never got a response to this:
> + 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.
Finally, how about using an extra set of parens to silence the warning? It's
harder to typo, and we have some precedent for that.
Jordan
On May 13, 2014, at 3:27 , Anders Rönnholm <[email protected]> wrote:
> Pinging
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 27 mars 2014 11:09
> Till: Jordan Rose
> Cc: [email protected]; Daniel Marjamäki
> Ämne: SV: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
>
> New patch with new diagnostic message. I couldn't come up with a better
> wording so i'm using your suggestion. I don't know of a good way to silence
> the warning.
>
> I removed the check for HasSideEffects previously but had to take back. I
> noticed that the patch triggered some false positives without it.
>
> //Anders
>
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>
> Mobile: +46 (0)70 912 42 54
> E-mail: [email protected]
>
> www.evidente.se
>
> ________________________________________
> Från: Jordan Rose [[email protected]]
> Skickat: den 31 januari 2014 18:50
> Till: Anders Rönnholm
> Cc: [email protected]; Daniel Marjamäki
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
>
> 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
> <[email protected]<mailto:[email protected]>> wrote:
>
> Hi,
>
> New one with comments handled.
>
> ________________________________________
> Från: Jordan Rose [[email protected]<mailto:[email protected]>]
> Skickat: den 20 december 2013 19:15
> Till: Anders Rönnholm
> Cc: [email protected]<mailto:[email protected]>; 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
> <[email protected]<mailto:[email protected]><mailto:[email protected]>>
> 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.
>
> <sizeofonexpression.diff>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
