rsmith added a comment. In https://reviews.llvm.org/D52137#1236065, @Quuxplusone wrote:
> In https://reviews.llvm.org/D52137#1236053, @rsmith wrote: > > > I think we can and should do better about false positives here. If you move > > this check to SemaChecking, you can produce the warning in a context where > > you know what the final type is -- I don't think there's any reason to warn > > if the final type is signed and no wider than the promoted type of the > > negation. > > > I share your general concern about false positives, but in the specific case > you mentioned— > > void use(int16_t x) > uint8_t u8 = 1; > use(-u8); > > > —I think it'd be surprising to maybe 50% of average programmers that `x`'s > received value wasn't `int16_t(255)` but rather `int16_t(-1)`. You may be right. But 50% is a *far* too high false positive rate for a Clang warning, so the conclusion should be that we do not warn on that case. Compare that to: unsigned int n = //... long m = -n; // almost certainly a bug (if long is wider than int) or if (-n < x && x < n) // almost certainly a bug If 50% of people turn this warning off because it uselessly warns on non-bugs, we are doing our users a disservice. We need to have a clear idea of what classes of bugs we want to catch here. Unary negation applied to an unsigned type does not deserve a warning by itself, but there are certainly some contexts in which we should warn. The interesting part is identifying them. ================ Comment at: test/Sema/unary-minus-unsigned.c:8 + + unsigned int a2 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}} + unsigned long b2 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}} ---------------- It's unreasonable to warn on this. The user clearly meant the result type to be unsigned: they wrote that type on the left-hand side. ================ Comment at: test/Sema/unary-minus-unsigned.c:9-10 + unsigned int a2 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}} + unsigned long b2 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}} + unsigned long long c2 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}} + ---------------- For these cases, we should warn that the high order bits are zeroes, since that's the surprising thing, not that the result is unsigned (which was obviously intended). ================ Comment at: test/Sema/unary-minus-unsigned.c:12 + + int a3 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}} + long b3 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}} ---------------- I don't see any point in warning here. We guarantee that the result is exactly the same as that of ``` int a3 = -(int)a; ``` ================ Comment at: test/Sema/unary-minus-unsigned.c:13-14 + int a3 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}} + long b3 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}} + long long c3 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}} + ---------------- This is a much less helpful warning than we could give; a better warning would be that the resulting value is always non-negative. (With appropriate modifications when `sizeof(int)` == `sizeof(long)`.) ================ Comment at: test/Sema/unary-minus-unsigned.c:16-18 + unsigned int a4 = -1u; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}} + unsigned long b4 = -1ul; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}} + unsigned long long c4 = -1ull; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}} ---------------- Warning on these is unreasonable. These are idiomatic ways of writing certain unsigned constants. https://reviews.llvm.org/D52137 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
