[PATCH] D52137: Added warning for unary minus used with unsigned type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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