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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
