[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Adam Schnitzer]

Anna, working on the StreamChecker sounds like a great idea. In terms of
work left to do on it, I'm assuming it's mostly cleanup and testing? It
also seems like the pattern of ensuring a resource should be opened/closed
once seems relevant beyond streams. Maybe it would make sense to refactor
it to include a generic base checker which could be reused?

Also, I'm not sure if anyone has had a chance to take a look yet, but I've
been having a little problem
<http://llvm.org/bugs/show_bug.cgi?id=15774>with the
BoolAssignmentChecker. I'm a bit stuck at the moment, but would be
interested in extending that as well.

Adam

On Thu, Apr 18, 2013 at 2:56 PM, Anna Zaks <ganna at apple.com> wrote:

> On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <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?
>
>
> Adam
>
>
>
>
>>
>>
>
>> 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/20130419/27f6ffb1/attachment.html>