[clang-tools-extra] a8120a7 - [clang-tidy] Ignore narrowing conversions in case of bitfields
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 29 00:57:17 PST 2021
Author: Balazs Benics
Date: 2021-11-29T09:56:43+01:00
New Revision: a8120a771143c15480b508c19a14c0c85a36378c
URL: https://github.com/llvm/llvm-project/commit/a8120a771143c15480b508c19a14c0c85a36378c
DIFF: https://github.com/llvm/llvm-project/commit/a8120a771143c15480b508c19a14c0c85a36378c.diff
LOG: [clang-tidy] Ignore narrowing conversions in case of bitfields
Bitfields are special. Due to integral promotion [conv.prom/5] bitfield
member access expressions are frequently wrapped by an implicit cast to
`int` if that type can represent all the values of the bitfield.
Consider these examples:
struct SmallBitfield { unsigned int id : 4; };
x.id & 1; (case-1)
x.id & 1u; (case-2)
x.id << 1u; (case-3)
(unsigned)x.id << 1; (case-4)
Due to the promotion rules, we would get a warning for case-1. It's
debatable how useful this is, but the user at least has a convenient way
of //fixing// it by adding the `u` unsigned-suffix to the literal as
demonstrated by case-2. However, this won't work for shift operators like
the one in case-3. In case of a normal binary operator, both operands
contribute to the result type. However, the type of the shift expression is
the promoted type of the left operand. One could still suppress this
superfluous warning by explicitly casting the bitfield member access as
case-4 demonstrates, but why? The compiler already knew that the value from
the member access should safely fit into an `int`, why do we have this
warning in the first place? So, hereby we suppress this specific scenario,
when a bitfield's value is implicitly cast to int (likely due to integral
promotion).
Note that the bitshift operation might invoke unspecified/undefined
behavior, but that's another topic, this checker is about detecting
conversion-related defects.
Example AST for `x.id << 1`:
BinaryOperator 'int' '<<'
|-ImplicitCastExpr 'int' <IntegralCast>
| `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
| `-MemberExpr 'unsigned int' lvalue bitfield .id
| `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
`-IntegerLiteral 'int' 1
Reviewed By: courbet
Differential Revision: https://reviews.llvm.org/D114105
Added:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
Modified:
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
index 5e59462b2900b..4d67c33361c17 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -58,6 +58,14 @@ void NarrowingConversionsCheck::storeOptions(
Options.store(Opts, "PedanticMode", PedanticMode);
}
+AST_MATCHER(FieldDecl, hasIntBitwidth) {
+ assert(Node.isBitField());
+ const ASTContext &Ctx = Node.getASTContext();
+ unsigned IntBitWidth = Ctx.getIntWidth(Ctx.IntTy);
+ unsigned CurrentBitWidth = Node.getBitWidthValue(Ctx);
+ return IntBitWidth == CurrentBitWidth;
+}
+
void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
// ceil() and floor() are guaranteed to return integers, even though the type
// is not integral.
@@ -83,6 +91,46 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
binaryOperator(hasOperands(IsConversionFromIgnoredType,
hasType(isInteger()))));
+ // Bitfields are special. Due to integral promotion [conv.prom/5] bitfield
+ // member access expressions are frequently wrapped by an implicit cast to
+ // `int` if that type can represent all the values of the bitfield.
+ //
+ // Consider these examples:
+ // struct SmallBitfield { unsigned int id : 4; };
+ // x.id & 1; (case-1)
+ // x.id & 1u; (case-2)
+ // x.id << 1u; (case-3)
+ // (unsigned)x.id << 1; (case-4)
+ //
+ // Due to the promotion rules, we would get a warning for case-1. It's
+ // debatable how useful this is, but the user at least has a convenient way of
+ // //fixing// it by adding the `u` unsigned-suffix to the literal as
+ // demonstrated by case-2. However, this won't work for shift operators like
+ // the one in case-3. In case of a normal binary operator, both operands
+ // contribute to the result type. However, the type of the shift expression is
+ // the promoted type of the left operand. One could still suppress this
+ // superfluous warning by explicitly casting the bitfield member access as
+ // case-4 demonstrates, but why? The compiler already knew that the value from
+ // the member access should safely fit into an `int`, why do we have this
+ // warning in the first place? So, hereby we suppress this specific scenario.
+ //
+ // Note that the bitshift operation might invoke unspecified/undefined
+ // behavior, but that's another topic, this checker is about detecting
+ // conversion-related defects.
+ //
+ // Example AST for `x.id << 1`:
+ // BinaryOperator 'int' '<<'
+ // |-ImplicitCastExpr 'int' <IntegralCast>
+ // | `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
+ // | `-MemberExpr 'unsigned int' lvalue bitfield .id
+ // | `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
+ // `-IntegerLiteral 'int' 1
+ const auto ImplicitIntWidenedBitfieldValue = implicitCastExpr(
+ hasCastKind(CK_IntegralCast), hasType(asString("int")),
+ has(castExpr(hasCastKind(CK_LValueToRValue),
+ has(ignoringParens(memberExpr(hasDeclaration(
+ fieldDecl(isBitField(), unless(hasIntBitwidth())))))))));
+
// Casts:
// i = 0.5;
// void f(int); f(0.5);
@@ -100,7 +148,8 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
IgnoreConversionFromTypes.empty()
? castExpr()
: castExpr(unless(hasSourceExpression(
- IsIgnoredTypeTwoLevelsDeep))))
+ IsIgnoredTypeTwoLevelsDeep))),
+ unless(ImplicitIntWidenedBitfieldValue))
.bind("cast")),
this);
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
new file mode 100644
index 0000000000000..36fde38202efc
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
@@ -0,0 +1,203 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -std=c++17 -- -target x86_64-unknown-linux
+
+#define CHAR_BITS 8
+static_assert(sizeof(unsigned int) == 32 / CHAR_BITS);
+
+template <typename T, typename U>
+struct is_same {
+ static constexpr bool value = false;
+};
+template <typename T>
+struct is_same<T, T> {
+ static constexpr bool value = true;
+};
+
+template <typename T, typename U>
+static constexpr bool is_same_v = is_same<T, U>::value;
+
+struct NoBitfield {
+ unsigned int id;
+};
+struct SmallBitfield {
+ unsigned int id : 4;
+};
+
+struct BigBitfield {
+ unsigned int id : 31;
+};
+struct CompleteBitfield {
+ unsigned int id : 32;
+};
+
+int example_warning(unsigned x) {
+ // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ return x;
+}
+
+void test_binary_and(SmallBitfield x) {
+ static_assert(is_same_v<decltype(x.id & 1), int>);
+ static_assert(is_same_v<decltype(x.id & 1u), unsigned>);
+
+ x.id & 1;
+ x.id & 1u;
+
+ 1 & x.id;
+ 1u & x.id;
+}
+
+void test_binary_or(SmallBitfield x) {
+ static_assert(is_same_v<decltype(x.id | 1), int>);
+ static_assert(is_same_v<decltype(x.id | 1u), unsigned>);
+
+ x.id | 1;
+ x.id | 1u;
+
+ 1 | x.id;
+ 1u | x.id;
+}
+
+template <typename T>
+void take(T);
+
+void test_parameter_passing(NoBitfield x) {
+ take<char>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined
+ take<short>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined
+ take<unsigned>(x.id);
+ take<int>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined
+ take<long>(x.id);
+ take<long long>(x.id);
+}
+
+void test_parameter_passing(SmallBitfield x) {
+ take<char>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined
+ take<short>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined
+ take<unsigned>(x.id);
+ take<int>(x.id); // no-warning
+ take<long>(x.id);
+ take<long long>(x.id);
+}
+
+void test_parameter_passing(BigBitfield x) {
+ take<char>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined
+ take<short>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined
+ take<unsigned>(x.id);
+ take<int>(x.id); // no-warning
+ take<long>(x.id);
+ take<long long>(x.id);
+}
+
+void test_parameter_passing(CompleteBitfield x) {
+ take<char>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined
+ take<short>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined
+ take<unsigned>(x.id);
+ take<int>(x.id);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined
+ take<long>(x.id);
+ take<long long>(x.id);
+}
+
+void test(NoBitfield x) {
+ static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+ static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+ static_assert(is_same_v<decltype(x.id + 1), unsigned>);
+ static_assert(is_same_v<decltype(x.id + 1u), unsigned>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+ x.id + 1;
+ x.id + 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+ 1 + x.id;
+ 1u + x.id;
+}
+
+void test(SmallBitfield x) {
+ static_assert(is_same_v<decltype(x.id << 1), int>);
+ static_assert(is_same_v<decltype(x.id << 1u), int>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ x.id + 1;
+ x.id + 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ 1 + x.id;
+ 1u + x.id;
+}
+
+void test(BigBitfield x) {
+ static_assert(is_same_v<decltype(x.id << 1), int>);
+ static_assert(is_same_v<decltype(x.id << 1u), int>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ x.id + 1;
+ x.id + 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ 1 + x.id;
+ 1u + x.id;
+}
+
+void test(CompleteBitfield x) {
+ static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+ static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ x.id + 1;
+ x.id + 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ 1 + x.id;
+ 1u + x.id;
+}
+
+void test_parens(SmallBitfield x) {
+ static_assert(is_same_v<decltype(x.id << (2)), int>);
+ static_assert(is_same_v<decltype(((x.id)) << (2)), int>);
+ x.id << (2);
+ ((x.id)) << (2);
+
+ static_assert(is_same_v<decltype((2) << x.id), int>);
+ static_assert(is_same_v<decltype((2) << ((x.id))), int>);
+ (2) << x.id;
+ (2) << ((x.id));
+}
More information about the cfe-commits
mailing list