[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
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>
More information about the cfe-commits
mailing list