[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Anton Yartsev]

On 18.04.2013 3:15, Jordan Rose wrote:
> 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?
Hi, Jordan, hi Adam.

Unfortunately I failed to find the source that inspired the idea of this 
checker.
Probably the initial idea was even to detect "=+" written instead of "+=":

void test() {
    unsigned int i = 7;
    i =+ i;  // did you mean += ?
    i =+ 7;  // did you mean += ?
}

Anyway, this is an idea how to make the checker more useful.


Yes, warn when promoting a small unsigned looks like potentially most 
useful.
However haven't imagined any realistic pattern, when the unary + is used 
deliberately for something other then promotion so far.


>
>
> /(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 
> <mailto: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>
>


-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130418/2e6585a1/attachment.html>