[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Anton Yartsev]

On 18.04.2013 22:56, Anna Zaks wrote:
> On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <adamschn at umich.edu 
> <mailto:adamschn at umich.edu>> wrote:
>
>> Jordan, Thank you very much for the feedback, I have a few comments.
>>
>>     /(1) Despite being on our list, this is probably better suited to
>>     a compiler warning./
>>
>>
>> I agree, this warning might be better as a compiler warning. I chose 
>> to implement this checker as a mainly to learn a bit about the 
>> analyzer. This one was on the list and seemed like a good place to 
>> get started.
>
> Sorry for having an under-specified checker in the list!
>
>>     /(2) Despite being on our list, "unsigned" isn't actually the
>>     interesting thing to check./
>>
>>
>> When I was reading the checker suggestion, I interpreted the purpose 
>> to be a more conservative version of a check for unary '+', which, 
>> arguably, is often dead code. For example, I have seen structures 
>> like this fairly commonly:
>>
>> int array[] = {
>>   -3,
>>   -2,
>>   -1,
>>   +1
>> };
>>
>> Where the '+' is used for alignment, which we wouldn't want to warn 
>> about. However, if that array was changed to unsigned, it might be a 
>> legitimate warning. I thought the assumption was there's at least a 
>> decent chance a unary '+' on unsigned is dead code. The place where I 
>> most commonly it pop up was legitimate:
>>
>> char a = 'A';
>> cout << a << " ";  // print A
>> cout << +a;  // prints numerical value of 'A'
>>
>
> This is in line with what Jordan had mentioned. If we are writing a 
> checker/warning that warns on redundant operations (or operations that 
> have no effect), we would not warn in this case as there will be a 
> promotion.
>
> It should be possible to write a check/warning that finds cases where 
> the unary plus has no effect by examining the AST. It could be a 
> candidate for a compiler warning, since the check could be fast and 
> does not require path-sensitive program exploration. Generally, 
> compiler warnings are better because they reach more users. If you are 
> interested, you could reach out to the clang community and see if 
> there is an interest in such a warning. You could also write it as a 
> checker first, see what is the false positive rate and rewrite this as 
> a compiler warning is it seems useful.
>
>> But I hadn't considered the checker was intended to target idempotent 
>> or erroneous promotions. If that is the intent, then it seems 
>> challenging to decide whether an expression is dead code, or to 
>> "force a load", as you put it.
>>
>>
>>     /(3) Macros and templates make this tricky./
>>
>>
>> I thought the that this might have been the reason why the checker 
>> was listed as a potential checker, rather than a compiler warning. It 
>> does seem like a fairly "noisy" warning. I have run it through some 
>> student code. Unfortunately all warnings it produced were false 
>> positives, with the exception of one situation similar to the one above.
>>
>
> If you are interested in writing the warning, you could look at your 
> results and see if the suggested changes would get rid of the false 
> positives.
>
>>
>> At this point, I'd be fine with throwing this checker out, as its 
>> utility does seem quite limited. If anyone has any ideas on how this 
>> checker can be improved to be more useful, I would be interested to hear.
>>
>> On an unrelated note, do you have any recommendations for what might 
>> be a approachable second checker?
>
> I think the i++ checker that you've proposed originally would be good. 
>  You could also productize the StreamChecker, which would be 
> path-sensitive and not too difficult. Note sure if anyone else is 
> working on that..
>
> Jordan, Anton, what do you think?

Agree with Anna. If you want to get familiar with the analyzer I advise 
you to pick something path-sensitive like StreamChecker, or 
different.NullDerefStmtOrder.
Unfortunately when I compilated the list I had no sufficient experience 
in checker writing (path-sensitive specifically) so the most of proposed 
examples and checkers are targeting simple AST-based checks.
I intend to continue working on the existing and proposed checkers lists 
with new experience gained after completing with NewDelete checker and 
related.


Concerning ideas on how the UnaryPlusChecker checker could be improved. 
What about detecting "=+" written instead of "+=" as in the following test:

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

What do you think?

>
>>
>> Adam
>>
>>
>>
>>     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/20130419/2724b9c6/attachment.html>