<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 18.04.2013 3:15, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:EC3774C6-B909-46C6-9D83-5B95A7BC49FA@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div>Hi, Adam. Thanks for putting this together, but while looking
it over I had several thoughts:</div>
<div><br>
</div>
<div>(1) Despite being on our list, this is probably better suited
to a compiler warning.</div>
<div>(2) Despite being on our list, "unsigned" isn't actually the
interesting thing to check. (CCing Anton, who compiled the
original list.)</div>
<div>(3) Macros and templates make this tricky.</div>
<div><br>
</div>
<div>I'll try to expand on each of these.</div>
<div><br>
</div>
<div><i>(1) Despite being on our list, this is probably better
suited to a compiler warning.</i></div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div><br>
</div>
<div>
<div><i>(2) Despite being on our list, "unsigned" isn't actually
the interesting thing to check.</i></div>
</div>
<div><br>
</div>
<div>According to C++11 [expr.unary.op]p7:</div>
<div><br>
</div>
<div>
<div class="page" title="Page 119">
<div class="layoutArea">
<div class="column"> </div>
</div>
</div>
</div>
<blockquote type="cite">
<div class="page" title="Page 119">
<div class="layoutArea">
<div class="column"><span style="font-size: 10pt;
font-family: LMRoman10; ">The operand of the unary </span><span
style="font-size: 10pt; font-family: LMMono10; ">+ </span><span
style="font-size: 10pt; font-family: LMRoman10; ">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. </span>
<ol start="5" style="list-style-type: none">
</ol>
</div>
</div>
</div>
</blockquote>
<div>So '+' has three effects:</div>
<div>- force a load – in C and C++ 'p' is an lvalue, but '+p' is
an rvalue.</div>
<div>- perform promotions</div>
<div>- operator overloading (unrelated)</div>
<div><br>
</div>
<div>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').</div>
<div><br>
</div>
<div>Thinking about it now, the last one actually seems like the
most useful warning. Anton, do you remember the intent here?</div>
</blockquote>
Hi, Jordan, hi Adam.<br>
<br>
Unfortunately I failed to find the source that inspired the idea of
this checker. <br>
Probably the initial idea was even to detect "=+" written instead of
"+=":<br>
<br>
void test() {<br>
unsigned int i = 7;<br>
i =+ i; // did you mean += ?<br>
i =+ 7; // did you mean += ?<br>
}<br>
<br>
Anyway, this is an idea how to make the checker more useful.<br>
<br>
<br>
Yes, warn when promoting a small unsigned looks like potentially
most useful.<br>
However haven't imagined any realistic pattern, when the unary + is
used deliberately for something other then promotion so far.<br>
<br>
<br>
<blockquote
cite="mid:EC3774C6-B909-46C6-9D83-5B95A7BC49FA@apple.com"
type="cite">
<div><br>
</div>
<div><br>
</div>
<div>
<div><i>(3) Macros and templates make this tricky.</i></div>
</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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?</div>
<div><br>
</div>
<div>Jordan</div>
<div><br>
</div>
<br>
<div>
<div>On Apr 12, 2013, at 23:53 , 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">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>
<span><UnaryPlusChecker.patch></span></blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>