On 18.04.2013 3:15, Jordan Rose wrote:
Hi, Adam. Thanks for putting this together, but while looking it over
I had several thoughts:
(1) Despite being on our list, this is probably better suited to a
compiler warning.
(2) Despite being on our list, "unsigned" isn't actually the
interesting thing to check. (CCing Anton, who compiled the original list.)
(3) Macros and templates make this tricky.
I'll try to expand on each of these.
/(1) Despite being on our list, this is probably better suited to a
compiler warning./
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).
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.
/(2) Despite being on our list, "unsigned" isn't actually the
interesting thing to check./
According to C++11 [expr.unary.op]p7:
The operand of the unary + 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.
So '+' has three effects:
- force a load – in C and C++ 'p' is an lvalue, but '+p' is an rvalue.
- perform promotions
- operator overloading (unrelated)
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').
Thinking about it now, the last one actually seems like the most
useful warning. Anton, do you remember the intent here?
Hi, Jordan, hi Adam.
Unfortunately I failed to find the source that inspired the idea of this
checker.
Probably the initial idea was even to detect "=+" written instead of "+=":
void test() {
unsigned int i = 7;
i =+ i; // did you mean += ?
i =+ 7; // did you mean += ?
}
Anyway, this is an idea how to make the checker more useful.
Yes, warn when promoting a small unsigned looks like potentially most
useful.
However haven't imagined any realistic pattern, when the unary + is used
deliberately for something other then promotion so far.
/(3) Macros and templates make this tricky./
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.
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?
Jordan
On Apr 12, 2013, at 23:53 , Adam Schnitzer <[email protected]
<mailto:[email protected]>> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
