Re: [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.ronnh...@evidente.se> wrote: Hi, New one with comments handled. ________________________________________ Från: Jordan Rose [jordan_r...@apple.com] Skickat: den 20 december 2013 19:15 Till: Anders Rönnholm Cc: cfe-commits@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.ronnh...@evidente.se<mailto:anders.ronnh...@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.

sizeofonexpression.diff
Description: Binary data

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits