[PATCH] [Analyzer] Implementation of UnaryPlusChecker

Anton Yartsev anton.yartsev at gmail.com
Wed Apr 17 21:47:51 PDT 2013

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 
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 
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>


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

More information about the cfe-commits mailing list