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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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]
----------------
gchatelet wrote:
> 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?
I see no reason to warn on a literal value that isn't changed as a result of the notional narrowing. The standard clearly defines the behavior, so I think that would be needlessly chatty.


================
Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:99
+void narrow_integer_to_floating() {
+  {
+    long long ll; // 64 bits
----------------
gchatelet wrote:
> 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
Ah, I missed that, thank you!


================
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;
----------------
gchatelet wrote:
> 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.
Ah, thank you! Can you also pass `-funsigned-char` to test the behavior is as expected?


================
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;
----------------
gchatelet wrote:
> 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`
Please add a test for another target where this is not the case.


================
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;
----------------
gchatelet wrote:
> 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
unsigned long is not 64-bit on all targets, and that should be tested. https://godbolt.org/z/VQx-Yy


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488