From ba1a73e0220937e26618ce0417a7aeadd0ed3792 Mon Sep 17 00:00:00 2001 From: Congcong Cai Date: Mon, 18 Nov 2024 15:09:34 +0800 Subject: [PATCH 1/2] [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 45fef9471d521..a053aa1544e8e 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 f967dfabd1c94..50dd594d17763 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 04260e75aa558..eb9539a6c25b1 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 36fde38202efc..7c5c38321ae94 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 0000000000000..32cf53aa24615 --- /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 fb5c7e36eeb0d..36f0f2a94c5ab 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 91e908f535a0d..3a7e045068c1f 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 cb19ed78cce8a..f5967c8a5e7ad 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 dcf1848a30f66..f63a758373a62 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 f58de65f04232..435f333ad180d 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 6bd437f98d44c..dcce3864feedf 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 29b38e74e1a22..6c7993cb51b94 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 From 1a6a3c4ba9d8bc2a424a9c24272c531a5316ac6e Mon Sep 17 00:00:00 2001 From: Congcong Cai Date: Tue, 26 Nov 2024 11:29:32 +0800 Subject: [PATCH 2/2] refactor implement --- .../NarrowingConversionsCheck.cpp | 22 ++++++++++--------- .../NarrowingConversionsCheck.h | 3 ++- .../narrowing-conversions.rst | 6 +++-- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp index a053aa1544e8e..9dd97c128cfb8 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -19,6 +19,7 @@ #include "llvm/ADT/SmallVector.h" #include +#include using namespace clang::ast_matchers; @@ -48,7 +49,7 @@ NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), WarnOnIntegerNarrowingConversion( - Options.get("WarnOnIntegerNarrowingConversion", true)), + Options.get("WarnOnIntegerNarrowingConversion")), WarnOnIntegerToFloatingPointNarrowingConversion( Options.get("WarnOnIntegerToFloatingPointNarrowingConversion", true)), WarnOnFloatingPointNarrowingConversion( @@ -61,8 +62,9 @@ NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name, void NarrowingConversionsCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "WarnOnIntegerNarrowingConversion", - WarnOnIntegerNarrowingConversion); + if (WarnOnIntegerNarrowingConversion.has_value()) + Options.store(Opts, "WarnOnIntegerNarrowingConversion", + WarnOnIntegerNarrowingConversion.value()); Options.store(Opts, "WarnOnIntegerToFloatingPointNarrowingConversion", WarnOnIntegerToFloatingPointNarrowingConversion); Options.store(Opts, "WarnOnFloatingPointNarrowingConversion", @@ -392,13 +394,13 @@ void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context, SourceLocation SourceLoc, 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; + // 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. + const bool ActualWarnOnIntegerNarrowingConversion = + WarnOnIntegerNarrowingConversion.value_or(!getLangOpts().CPlusPlus20); + if (ActualWarnOnIntegerNarrowingConversion) { const BuiltinType *ToType = getBuiltinType(Lhs); // From [conv.integral] before C++20: // Conversions to unsigned integer is well defined so no warning is issued. diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h index 1add40b91778a..cd859a88e7084 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H #include "../ClangTidyCheck.h" +#include namespace clang::tidy::cppcoreguidelines { @@ -95,7 +96,7 @@ class NarrowingConversionsCheck : public ClangTidyCheck { const BuiltinType &FromType, const BuiltinType &ToType) const; - const bool WarnOnIntegerNarrowingConversion; + const std::optional WarnOnIntegerNarrowingConversion; const bool WarnOnIntegerToFloatingPointNarrowingConversion; const bool WarnOnFloatingPointNarrowingConversion; const bool WarnWithinTemplateInstantiation; 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 eb9539a6c25b1..eb645e8138473 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 before C++20 (e.g. ``char`` to ``unsigned char``) + - an integer to a narrower integer (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, @@ -34,7 +34,9 @@ Options .. option:: WarnOnIntegerNarrowingConversion When `true`, the check will warn on narrowing integer conversion - (e.g. ``int`` to ``size_t``). `true` by default. + (e.g. ``int`` to ``size_t``). + Before C++20 `true` by default. + Since C++20 `false` by default. .. option:: WarnOnIntegerToFloatingPointNarrowingConversion