[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Adam Schnitzer]

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.


> *(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'

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.


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?

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/20130417/772bf7ee/attachment.html>