[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Jordan Rose]

Hi, Adam. Thanks for putting this together, but while looking it over I had several thoughts:

(1) Despite being on our list, this is probably better suited to a compiler warning.
(2) Despite being on our list, "unsigned" isn't actually the interesting thing to check. (CCing Anton, who compiled the original list.)
(3) Macros and templates make this tricky.

I'll try to expand on each of these.

(1) Despite being on our list, this is probably better suited to a compiler warning.

Syntactic checks are much less expensive that path-sensitive checks, but in this case there's actually no clever syntactic check that needs to happen, either—this is a property that you can check right when the UnaryOperator expression is formed. It's also very straightforward (though see (2) below).

Compiler warnings are somewhat different from analyzer warnings, but if you wanted to take a stab at it the right place to put this would be Sema::CreateBuiltinUnaryOp, in SemaExpr.cpp.


(2) Despite being on our list, "unsigned" isn't actually the interesting thing to check.

According to C++11 [expr.unary.op]p7:

> The operand of the unary + operator shall have arithmetic, unscoped enumeration, or pointer type and the result is the value of the argument. Integral promotion is performed on integral or enumeration operands. The type of the result is the type of the promoted operand. 
So '+' has three effects:
- force a load – in C and C++ 'p' is an lvalue, but '+p' is an rvalue.
- perform promotions
- operator overloading (unrelated)

None of these actually have to do with unsigned. I'm not sure whether the original idea was to avoid pretending an unsigned variable is signed, or to warn when performing an idempotent promotion (which is true for anything at least as big as 'int'), or to warn when promoting a small unsigned variable (which would get promoted to 'int', not 'unsigned').

Thinking about it now, the last one actually seems like the most useful warning. Anton, do you remember the intent here?


(3) Macros and templates make this tricky.

Hopefully this one is obvious in hindsight, but of course we shouldn't warn about "dead" code that might behave differently in different contexts. For the "dangerous promotion" warning this gets a little fuzzier, but it's definitely easy to start conservative, and enable this in more cases if it proves more useful than noise.

One thing about compiler warnings is that they do have a higher level of exposure than analyzer checks. Have you run this over any existing codebases? What's the true positive : false positive ratio like?

Jordan


On Apr 12, 2013, at 23:53 , Adam Schnitzer <adamschn at umich.edu> wrote:

> This patch is an implementation of the proposed "different.UnaryPlusWithUnsigned", which I implemented
> as "alpha.deadcode.UnaryPlusWithUnsigned".
> 
> It is implemented as a simple AST checker. However, it seems that unary '+' is often removed from the AST
> as dead code. So some of the basic test cases don't work.
> 
> This is my first (real) patch, so any feedback or criticism is appreciated.
> 
> Adam Schnitzer
> <UnaryPlusChecker.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130417/5b919af8/attachment.html>