[PATCH] D42728: Add more warnings for implict conversions (e.g. double truncation to float).

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

Hmm.  I think if you're going to push for this, you need to put more time into making sure that the diagnostics are good, and most them seem pretty questionable.



================
Comment at: test/OpenMP/teams_distribute_simd_loop_messages.cpp:48
 // expected-error at +1 {{expression must have integral or unscoped enumeration type, not 'double'}}
-  for (long long i = 0; i < 10; i += 1.5) {
+  for (long long i = 0; i < 10; i += 1.5) { // expected-warning {{implicit conversion from 'double' to 'long long' changes value from 1.5 to 1}}
     c[i] = a[i] + b[i];
----------------
This, and all the other warnings in these OpenMP tests, is actually really misleading.  It happens to be true that the result of this computation is effectively as if the value being added was just 1, but that's not what the diagnostic actually says, and it wouldn't be true for all operations or values.


================
Comment at: test/Sema/constant-conversion.c:76
 	f.twoBits1 &= ~1; // no-warning
-	f.twoBits2 &= ~2; // no-warning
+	f.twoBits2 &= ~2; // expected-warning {{implicit truncation from 'int' to bit-field changes value from -3 to 1}}
 }
----------------
This doesn't seem like a good warning.  The original code masks off the high bit of the value stored in the bit-field, which is a perfectly sensible operation to do.  It's actually exactly the expected pattern of code for idiomatic bit manipulation.


================
Comment at: test/Sema/conversion.c:362
   char c = 5;
-  f7676608(c *= q);
+  f7676608(c *= q); // expected-warning {{implicit conversion turns floating-point number into integer: 'float' to 'char'}}
 }
----------------
This diagnostic is okay.  It would be nice to somehow refer to the fact the the conversion is the conversion done at the end of the compound operation.

Although... it's not like it's unreasonable to multiply an integer by a float and then truncate back to an integer.  So there needs to be some way of suppressing this warning, and I'm not sure what it would be, because you can't add a cast without changing the meaning of the operation, so you'd have to spell out the LHS twice, which is really unfortunate.  We usually don't like to add warnings for code that might be the best way of expressing what it needs to express.


================
Comment at: test/Sema/shift.c:31
+  c <<= 999999; // expected-warning {{shift count >= width of type}} expected-warning {{implicit conversion from 'int' to 'char' changes value from 999999 to 63}}
+  c >>= 999999; // expected-warning {{shift count >= width of type}} expected-warning {{implicit conversion from 'int' to 'char' changes value from 999999 to 63}}
   c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}}
----------------
Okay, this is really a bad diagnostic, because the value 999999 is in no way getting truncated.


================
Comment at: test/SemaCXX/conversion.cpp:27
+    return sinf(a); // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}
+}
+
----------------
Are you implying that we don't currently warn in these cases?


================
Comment at: test/SemaCXX/null_in_arithmetic_ops.cpp:65
+  a |= NULL; // expected-warning{{use of NULL in arithmetic operation}} expected-warning {{implicit conversion of NULL constant to 'int'}}
+  a ^= NULL; // expected-warning{{use of NULL in arithmetic operation}} expected-warning {{implicit conversion of NULL constant to 'int'}}
 
----------------
I feel like these don't add much on top of the other warning.


https://reviews.llvm.org/D42728