<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 18.04.2013 22:56, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:08D3779A-D4B7-41D9-A9E0-D4C6B35955FC@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div>
<div>On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <<a
moz-do-not-send="true" href="mailto:adamschn@umich.edu">adamschn@umich.edu</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">Jordan, Thank you very much
for the feedback, I have a few comments.
<div><br>
</div>
<div>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left-width: 1px; border-left-color:
rgb(204, 204, 204); border-left-style: solid;
padding-left: 1ex; position: static; z-index: auto;">
<div style="word-wrap: break-word;">
<div><i>(1) Despite being on our list, this is
probably better suited to a compiler warning.</i></div>
<div><br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Sorry for having an under-specified checker in the list!</div>
<div><br>
</div>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left-width: 1px; border-left-color:
rgb(204, 204, 204); border-left-style: solid;
padding-left: 1ex; position: static; z-index: auto;">
<div style="word-wrap: break-word;">
<div><i>(2) Despite being on our list, "unsigned"
isn't actually the interesting thing to check.</i></div>
<div><br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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:</div>
<div><br>
</div>
<div>int array[] = {</div>
<div> -3,</div>
<div> -2,</div>
<div> -1,</div>
<div> +1</div>
<div>};</div>
<div><br>
</div>
<div>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:</div>
<div><br>
</div>
<div>char a = 'A';</div>
<div>cout << a << " "; // print A</div>
<div>cout << +a; // prints numerical value of 'A'</div>
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>
<div class="gmail_quote">
<div>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.</div>
<div><br>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left-width: 1px; border-left-color:
rgb(204, 204, 204); border-left-style: solid;
padding-left: 1ex;">
<div style="word-wrap: break-word;"><i>(3) Macros and
templates make this tricky.</i></div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<br>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>
<div class="gmail_quote">
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
<blockquote
cite="mid:08D3779A-D4B7-41D9-A9E0-D4C6B35955FC@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>
<div class="gmail_quote">
<div><br>
</div>
<div>On an unrelated note, do you have
any recommendations for what might be a approachable
second checker?</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
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..</div>
<div><br>
</div>
<div>Jordan, Anton, what do you think?</div>
</blockquote>
<br>
Agree with Anna. If you want to get familiar with the analyzer I
advise you to pick something path-sensitive like StreamChecker, or <span
class="name">different.NullDerefStmtOrder.</span><br>
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.<br>
I intend to continue working on the existing and proposed checkers
lists with new experience gained after completing with NewDelete
checker and related.<br>
<br>
<br>
Concerning ideas on how the UnaryPlusChecker checker could be
improved. What about detecting "=+" written instead of "+=" as in
the following test:<br>
<br>
void test() {<br>
unsigned int i = 7;<br>
i =+ i; // d you mean '+=' ?<br>
i =+ 7; // did you mean '+=' ?<br>
}<br>
<br>
What do you think?<br>
<br>
<blockquote
cite="mid:08D3779A-D4B7-41D9-A9E0-D4C6B35955FC@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>
<div class="gmail_quote">
<div><br>
</div>
<div>Adam</div>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left-width: 1px; border-left-color:
rgb(204, 204, 204); border-left-style: solid;
padding-left: 1ex;">
<div style="word-wrap: break-word;"> </div>
</blockquote>
<blockquote class="gmail_quote" style="margin: 0px 0px
0px 0.8ex; border-left-width: 1px; border-left-color:
rgb(204, 204, 204); border-left-style: solid;
padding-left: 1ex;">
<div style="word-wrap: break-word;"><br>
<div>
<div>
<div>On Apr 12, 2013, at 23:53 , Adam Schnitzer
<<a moz-do-not-send="true"
href="mailto:adamschn@umich.edu"
target="_blank">adamschn@umich.edu</a>>
wrote:</div>
<br>
</div>
<blockquote type="cite">
<div>This patch is an implementation of the
proposed "different.UnaryPlusWithUnsigned",
which I implemented
<div>as
"alpha.deadcode.UnaryPlusWithUnsigned".
<div><br>
</div>
<div>It is implemented as a simple AST
checker. However, it seems that unary '+'
is often removed from the AST</div>
</div>
<div>as dead code. So some of the basic test
cases don't work.</div>
<div><br>
</div>
<div>This is my first (real) patch, so any
feedback or criticism is appreciated.</div>
<div><br>
</div>
<div>Adam Schnitzer</div>
</div>
<span><UnaryPlusChecker.patch></span></blockquote>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>