[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

Thx for the review. I have two suggestions in the comments let me know what you think.



================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
+  f += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
----------------
aaron.ballman wrote:
> This is not a narrowing conversion -- there should be no diagnostic here. See [dcl.init.list]p7.
Thx for pointing this out.
I would like to keep the diagnostic (without mentioning the narrowing) because there is no reason to write `2.0` instead of `2.0f`. WDYT?


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:99
+void narrow_integer_to_floating() {
+  {
+    long long ll; // 64 bits
----------------
aaron.ballman wrote:
> Missing tests involving literals, which are not narrowing conversions. e.g., `float f = 1ULL;` should not diagnose.
This is handled on l.262


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:174
+  c = uc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned char' to 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  c = us;
----------------
aaron.ballman wrote:
> This is only true on architectures where `char` is defined to be `signed char` rather than `unsigned char`.
This is taken care of. The test fails when adding `-funsigned-char`.
The current test is executed with `-target x86_64-unknown-linux` which should imply `-fsigned-char`. Anyways, I'll add `-fsigned-char` to make sure this is obvious.


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:188
+  i = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' to 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ll;
----------------
aaron.ballman wrote:
> This is only narrowing if `sizeof(int) < sizeof(long)` which is not the case on all architectures.
Yes this is taken care of in the code, the size of the type must be smaller.
It works here because the test runs with `-target x86_64-unknown-linux`


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:211
+  ll = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'unsigned long' to 'long long' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  ll = ull;
----------------
aaron.ballman wrote:
> Whatttt? How does one narrow from an unsigned 32-bit number to a signed 64-bit number?
`unsigned long` is unsigned 64-bit so it does not fit in signed 64-bit.
https://godbolt.org/z/VnmG0s


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:228
+void narrow_constant_to_signed_integer_is_not_ok() {
+  char c1 = -128;
+  char c2 = 127;
----------------
aaron.ballman wrote:
> This may be narrowing on an implementation that defines `char` to be `unsigned`.
Same as previous comments. Would it make sense to create a different test with `-funsigned-char` to check these behaviors?


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:232
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant value -129 (0xFFFFFF7F) of type 'int' to 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  char c4 = 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from constant value 128 (0x00000080) of type 'int' to 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
----------------
aaron.ballman wrote:
> This may be fine on implementations that define `char` to be `unsigned`.
ditto


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488