[clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 #116591
Conversation
…ves when narrowing integer to signed integer in C++20
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesFull diff: https://github.com/llvm/llvm-project/pull/116591.diff 12 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
index 45fef9471d5211..a053aa1544e8e1 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -393,8 +393,14 @@ void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context,
const Expr &Lhs,
const Expr &Rhs) {
if (WarnOnIntegerNarrowingConversion) {
+ // From [conv.integral] since C++20
+ // The result is the unique value of the destination type that is congruent
+ // to the source integer modulo 2^N, where N is the width of the destination
+ // type.
+ if (getLangOpts().CPlusPlus20)
+ return;
const BuiltinType *ToType = getBuiltinType(Lhs);
- // From [conv.integral]p7.3.8:
+ // From [conv.integral] before C++20:
// Conversions to unsigned integer is well defined so no warning is issued.
// "The resulting value is the smallest unsigned value equal to the source
// value modulo 2^n where n is the number of bits used to represent the
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f967dfabd1c940..50dd594d177631 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -207,6 +207,10 @@ Changes in existing checks
<clang-tidy/checks/cppcoreguidelines/init-variables>` check by fixing the
insertion location for function pointers.
+- Fixed :doc:`cppcoreguidelines-narrowing-conversions
+ <clang-tidy/checks/cppcoreguidelines/narrowing-conversions>` check to avoid
+ false positives when narrowing integer to signed integer in C++20.
+
- Improved :doc:`cppcoreguidelines-prefer-member-initializer
<clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
avoid false positive when member initialization depends on a structured
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.rst
index 04260e75aa558f..eb9539a6c25b1c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.rst
@@ -12,7 +12,7 @@ This check implements `ES.46
from the C++ Core Guidelines.
We enforce only part of the guideline, more specifically, we flag narrowing conversions from:
- - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``)
+ - an integer to a narrower integer before C++20 (e.g. ``char`` to ``unsigned char``)
if WarnOnIntegerNarrowingConversion Option is set,
- an integer to a narrower floating-point (e.g. ``uint64_t`` to ``float``)
if WarnOnIntegerToFloatingPointNarrowingConversion Option is set,
@@ -89,7 +89,9 @@ the range [-2^31, 2^31-1].
You may have encountered messages like "narrowing conversion from 'unsigned int'
to signed type 'int' is implementation-defined".
-The C/C++ standard does not mandate two's complement for signed integers, and so
-the compiler is free to define what the semantics are for converting an unsigned
-integer to signed integer. Clang's implementation uses the two's complement
-format.
+Before C++20, the C/C++ standard does not mandate two's complement for signed
+integers, and so the compiler is free to define what the semantics are for
+converting an unsigned integer to signed integer. Clang's implementation uses
+the two's complement format.
+Since C++20, the C++ standard defines the conversion between all kinds of
+integers.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-bitfields.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-bitfields.cpp
index 36fde38202efcd..7c5c38321ae94b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-bitfields.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-bitfields.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
-// RUN: -std=c++17 -- -target x86_64-unknown-linux
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -std=c++17 \
+// RUN: -- -target x86_64-unknown-linux
#define CHAR_BITS 8
static_assert(sizeof(unsigned int) == 32 / CHAR_BITS);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-cxx20.cpp
new file mode 100644
index 00000000000000..32cf53aa24615b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-cxx20.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -std=c++20 -- -Wno-everything
+
+void narrow_integer_to_signed_integer_is_ok_since_cxx20() {
+ char c;
+ short s;
+ int i;
+ long l;
+ long long ll;
+
+ unsigned char uc;
+ unsigned short us;
+ unsigned int ui;
+ unsigned long ul;
+ unsigned long long ull;
+
+ c = c;
+ c = s;
+ c = i;
+ c = l;
+ c = ll;
+
+ c = uc;
+ c = us;
+ c = ui;
+ c = ul;
+ c = ull;
+
+ i = c;
+ i = s;
+ i = i;
+ i = l;
+ i = ll;
+
+ i = uc;
+ i = us;
+ i = ui;
+ i = ul;
+ i = ull;
+
+ ll = c;
+ ll = s;
+ ll = i;
+ ll = l;
+ ll = ll;
+
+ ll = uc;
+ ll = us;
+ ll = ui;
+ ll = ul;
+ ll = ull;
+}
+
+void narrow_constant_to_signed_integer_is_ok_since_cxx20() {
+ char c1 = -128;
+ char c2 = 127;
+ char c3 = -129;
+ char c4 = 128;
+
+ short s1 = -32768;
+ short s2 = 32767;
+ short s3 = -32769;
+ short s4 = 32768;
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-equivalentbitwidth-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-equivalentbitwidth-option.cpp
index fb5c7e36eeb0df..36f0f2a94c5ab1 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-equivalentbitwidth-option.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-equivalentbitwidth-option.cpp
@@ -1,7 +1,7 @@
-// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t --
-// RUN: %check_clang_tidy -check-suffix=DISABLED %s \
+// RUN: %check_clang_tidy -check-suffix=DISABLED %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t -- \
// RUN: -config='{CheckOptions: { \
// RUN: cppcoreguidelines-narrowing-conversions.WarnOnEquivalentBitWidth: 0}}'
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
index 91e908f535a0d4..3a7e045068c1fd 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
@@ -1,7 +1,7 @@
-// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t --
-// RUN: %check_clang_tidy -check-suffix=IGNORED %s \
+// RUN: %check_clang_tidy -check-suffix=IGNORED %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t -- \
// RUN: -config='{CheckOptions: { \
// RUN: cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes: "global_size_t;nested_size_type;long" \
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-intemplates-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-intemplates-option.cpp
index cb19ed78cce8a7..f5967c8a5e7ade 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-intemplates-option.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-intemplates-option.cpp
@@ -1,7 +1,7 @@
-// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t --
-// RUN: %check_clang_tidy -check-suffix=WARN %s \
+// RUN: %check_clang_tidy -check-suffix=WARN %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t -- \
// RUN: -config='{CheckOptions: { \
// RUN: cppcoreguidelines-narrowing-conversions.WarnWithinTemplateInstantiation: 1 \
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-long-is-32bits.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-long-is-32bits.cpp
index dcf1848a30f664..f63a758373a62c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-long-is-32bits.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-long-is-32bits.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
-// RUN: -- -- -target x86_64-unknown-linux -m32
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -std=c++17 \
+// RUN: -- -target x86_64-unknown-linux -m32
static_assert(sizeof(int) * 8 == 32, "int is 32-bits");
static_assert(sizeof(long) * 8 == 32, "long is 32-bits");
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowinginteger-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowinginteger-option.cpp
index f58de65f042328..435f333ad180d6 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowinginteger-option.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowinginteger-option.cpp
@@ -1,8 +1,8 @@
-// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t -- \
// RUN: -config='{CheckOptions: {cppcoreguidelines-narrowing-conversions.WarnOnIntegerNarrowingConversion: true}}'
-// RUN: %check_clang_tidy -check-suffix=DISABLED %s \
+// RUN: %check_clang_tidy -check-suffix=DISABLED %s -std=c++17 \
// RUN: cppcoreguidelines-narrowing-conversions %t -- \
// RUN: -config='{CheckOptions: {cppcoreguidelines-narrowing-conversions.WarnOnIntegerNarrowingConversion: false}}'
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-unsigned-char.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-unsigned-char.cpp
index 6bd437f98d44c5..dcce3864feedf3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-unsigned-char.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-unsigned-char.cpp
@@ -1,5 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
-// RUN: -- -- -target x86_64-unknown-linux -funsigned-char
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -std=c++17 \
+// RUN: -- -target x86_64-unknown-linux -funsigned-char
void narrow_integer_to_unsigned_integer_is_ok() {
signed char sc;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions.cpp
index 29b38e74e1a22d..6c7993cb51b941 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -std=c++17 \
// RUN: -config="{CheckOptions: { \
// RUN: cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion: false}}" \
// RUN: -- -target x86_64-unknown-linux -fsigned-char
|
|
My understanding of the guidelines is that the purpose of this rule is to avoid data loss (truncation) due to narrowing. In that sense, isn't this still a problem in C++20? Whether it well-defined behavior in C++20 or implementation-defined behavior pre-C++20 does not seem relevant to me. We could certainly adjust the warning message to avoid saying it's "implementation-defined" in C++20. |
|
I agree cppcoreguideline wants to check it. but since we have already allow the to unsigned cast later according to [conv.intergral] before c++20, I think here the behavior should be the same. we should either remove or accept both limitation. |
| if (getLangOpts().CPlusPlus20) | ||
| return; |
There was a problem hiding this comment.
Better would-be to change default value for WarnOnIntegerNarrowingConversion on C++20. We could use optional<bool> for that option, and if not set then it could assing value based on C++20 being used or not.
There was a problem hiding this comment.
thanks, it is better than current solution.
There was a problem hiding this comment.
do you think we should apply similar approach to unsigned integer target type also to make the behaviour similar?
but it can be done separately
I'm not sure I follow, could you point me to an example of this? When implementing guidelines, we must make sure we implement exactly what the guidelines say, and not make them less restrictive (at least not by default). The user expectation is that a C++ Core Guideline check will implement just that, not something else. I cannot see anything in the C++ Core Guidelines that says narrowing is OK in C++20 (since information is still lost). |
|
On the other hand the rule does not explicitly state that unsigned int -> int should be covered in the "Enforcement" section, so I guess one can argue these warnings are never required by the rule and as such we can decide what we want to do. |
| c = uc; | ||
| c = us; | ||
| c = ui; | ||
| c = ul; |
There was a problem hiding this comment.
The guidelines do mandate long/int -> char to be detected, regardless of C++ Standard
Flag all long->char (I suspect int->char is very common. Here be dragons! we need data).
|
Perhaps add another option |
It is in below the line I modified. if (ToType->isUnsignedInteger())
return; |
|
Maybe we should move this check to bugprone and use some configuration to fulfill guideline in cppcoreguidelines parts. WDYT? @carlosgalvezp @PiotrZSL |
+1
The cppcoreguideline check should implement what the guidelines say, but giving the user choice is a plus. There is no need to move the check to bugprone, because this check is already an alias from the cppcoreguidelines to bugprone. So instead, we could configure the alias in bugprone differently? Right now it is an unconfigured alias. |
|
We probably should have proper guidelines for aliases. IMO, there should only be a "base -> coding guideline" kind of alias relationship, not the other way around. Coding guidelines should "cherry-pick" (and possibly configure/harden/make more strict) base checks. So in that sense yes, it would make sense to move the check to bugprone and alias to cppcoreguidelines. But I think that's a separate issue.
Absolutely, I'm not arguing against that, I'm just saying that the default choice should be what the guidelines say. I still don't understand what problem we are trying to solve, however. My impression from the PR (please correct me if I'm wrong) is that we want to ignore integer narrowing conversions in C++20, simply because they are well-defined. I believe a lot of people (myself included) will disagree with that - like I wrote above, the problem still remains that data is lost, which is exactly what this check is preventing. If one wants to ignore these warnings, one can disable the check or disable warnings on integer conversions which the existing option. Why is that not sufficient? |
|
It is difference level of issue. if it is an implement defined thing, it should be disabled unrelated to which coding guideline they use. But if it is well defined behavior, then coding guideline just a coding guideline. It even does not allow this kind of code. short v(short a, short b) { return a + b; } // warning: narrowing conversion from 'int' to signed type 'short' is implementation-defined [cppcoreguidelines-narrowing-conversions] |
|
Actually cppcodingguideline require |
|
IMO, we should move this check to bugprone and provide 3 level's error.
For the first level, we emit warning for explicit and implicit cast. WDYT? @5chmidti @carlosgalvezp |
|
The move to bugprone makes sense to me. However, I'm not sure that conversions of integers to signed integers should be ignored-by-default since C++20. It may be well-defined behavior w.r.t. the resulting value, but the pattern is still prone to bugs. You will get a well-defined wrong value from C++20 on, instead of an implementation-defined wrong value.
That sounds like a good idea.
... and MSVC, and NVCC? I'd drop condition on the compilers behaving the same for targets. All implementation-defined behavior according to the standard sounds better than only those that are not agreed upon. There are also downstream versions of compilers for a few architectures (e.g., microcontrollers), and those may or may not follow the practice of their upstream counterparts (different targets).
I think that the bugprone check should also detect narrowing conversions for well-defined conversions because the check is primarily about loosing information (in my opinion), not that these narrowing conversions are implementation defined. Though that is also a core part of this check. |
…gprone-narrowing-conversions According to llvm#116591. > Coding guidelines should "cherry-pick" (and posddsibly configure/harden/make more strict) base checks. We should move narrowing conversion to bugprone and keep alias in cppcoreguidelines
…gprone-narrowing-conversions According to llvm#116591. > Coding guidelines should "cherry-pick" (and posddsibly configure/harden/make more strict) base checks. We should move narrowing conversion to bugprone and keep alias in cppcoreguidelines
HerrCai0907
deleted the
fix/false-postive-cxx20-narrowing-conversions-int-to-singed
branch
Since C++20, the result is the unique value of the destination type that is congruent to the source integer modulo 2^N, where N is the width of the destination type.
It is well-defined behavior even when type is signed. So it is no need to diagnose an implement-behavior in this cases.