[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
https://github.com/HerrCai0907 closed https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
5chmidti wrote: 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. --- > 1. really implement defined behavior -> The above example about float to > unsigned For the first level, we emit warning for explicit and implicit cast. That sounds like a good idea. > 2. standard implement defined behavior but at least gcc and clang in each > target has same behavior <- default ... 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). > 3. most strictly check according to cppcodingguideline. <- create alias in > this level 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. https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
HerrCai0907 wrote: Actually cppcodingguideline require `gsl` support, which mean the most of fixture in this check is still wrong. I see lots of people in project just use explicit cast instead of implicit cast to avoid this check. But it is totally wrong and mean nothing. Because in c++ standard, [conv.integral], [conv.double] are designed for all kinds of cast. Here is an example about target related behavior about float to unsigned. Even I use static_cast, bug still exists. https://godbolt.org/z/eTG1hfod8 https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
HerrCai0907 wrote:
It is difference level of issue. if it is an implement defined thing, it should
be disabled unrelated to which coding guideline. But if it is well defined
behavior, then coding guideline just a coding guideline. It even does not allow
this kind of code.
```c++
short v(short a, short b) { return a + b; } // warning: narrowing conversion
from 'int' to signed type 'short' is implementation-defined
[cppcoreguidelines-narrowing-conversions]
```
https://github.com/llvm/llvm-project/pull/116591
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
HerrCai0907 wrote: IMO, we should move this check to bugprone and provide 3 level's error. 1. really implement defined behavior -> The above example about float to unsigned 2. standard implement defined behavior but at least gcc and clang in each target has same behavior 3. most strictly check according to cppcodingguideline. For the first level, we emit warning for explicit and implicit cast. For the other level, we emit warning only for implicit cast. WDYT? @5chmidti @carlosgalvezp https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
carlosgalvezp wrote: 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. > but giving the user choice is a plus 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 achieve, 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? https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
5chmidti wrote: > 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. +1 > 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. > people can choose if they want to relax the rule or not. > Maybe we should move this check to bugprone and use some configuration to > fulfill guideline in cppcoreguidelines parts. 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. https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
HerrCai0907 wrote: Maybe we should move this check to bugprone and use some configuration to enforce guideline in cppcoreguidelines parts. WDYT? @carlosgalvezp @PiotrZSL https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
carlosgalvezp wrote: > since we have already allow the to unsigned cast later 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). https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
@@ -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;
HerrCai0907 wrote:
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
https://github.com/llvm/llvm-project/pull/116591
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
@@ -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;
HerrCai0907 wrote:
thanks, it is better than current solution.
https://github.com/llvm/llvm-project/pull/116591
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
@@ -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;
PiotrZSL wrote:
Better would-be to change default value for WarnOnIntegerNarrowingConversion on
C++20. We could use `optional` for that option, and if not set then it
could assing value based on C++20 being used or not.
https://github.com/llvm/llvm-project/pull/116591
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
https://github.com/PiotrZSL requested changes to this pull request. Consider just changing default value of WarnOnIntegerNarrowingConversion option. https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
https://github.com/HerrCai0907 edited https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
carlosgalvezp wrote: 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. https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
HerrCai0907 wrote: 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. https://github.com/llvm/llvm-project/pull/116591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Congcong Cai (HerrCai0907)
Changes
---
Full diff: https://github.com/llvm/llvm-project/pull/116591.diff
12 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
(+7-1)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
- (modified)
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/narrowing-conversions.rst
(+7-5)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-bitfields.cpp
(+2-2)
- (added)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-cxx20.cpp
(+63)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-equivalentbitwidth-option.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-intemplates-option.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-long-is-32bits.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowinginteger-option.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-unsigned-char.cpp
(+2-2)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions.cpp
(+1-1)
``diff
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
` check by fixing the
insertion location for function pointers.
+- Fixed :doc:`cppcoreguidelines-narrowing-conversions
+ ` check to avoid
+ false positives when narrowing integer to signed integer in C++20.
+
- Improved :doc:`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 ma
[clang-tools-extra] [clang-tidy] fix cppcoreguidelines-narrowing-conversions false positives when narrowing integer to signed integer in C++20 (PR #116591)
https://github.com/HerrCai0907 created
https://github.com/llvm/llvm-project/pull/116591
None
>From ba1a73e0220937e26618ce0417a7aeadd0ed3792 Mon Sep 17 00:00:00 2001
From: Congcong Cai
Date: Mon, 18 Nov 2024 15:09:34 +0800
Subject: [PATCH] [clang-tidy] fix cppcoreguidelines-narrowing-conversions
false positives when narrowing integer to signed integer in C++20
---
.../NarrowingConversionsCheck.cpp | 8 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../narrowing-conversions.rst | 12 ++--
.../narrowing-conversions-bitfields.cpp | 4 +-
.../narrowing-conversions-cxx20.cpp | 63 +++
...-conversions-equivalentbitwidth-option.cpp | 4 +-
...sions-ignoreconversionfromtypes-option.cpp | 4 +-
...rrowing-conversions-intemplates-option.cpp | 4 +-
.../narrowing-conversions-long-is-32bits.cpp | 4 +-
...ng-conversions-narrowinginteger-option.cpp | 4 +-
.../narrowing-conversions-unsigned-char.cpp | 4 +-
.../narrowing-conversions.cpp | 2 +-
12 files changed, 96 insertions(+), 21 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-cxx20.cpp
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
` check by fixing the
insertion location for function pointers.
+- Fixed :doc:`cppcoreguidelines-narrowing-conversions
+ ` check to avoid
+ false positives when narrowing integer to signed integer in C++20.
+
- Improved :doc:`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/narrowin
