This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8120a771143: [clang-tidy] Ignore narrowing conversions in
case of bitfields (authored by steakhal).
Changed prior to commit:
https://reviews.llvm.org/D114105?vs=389711&id=390275#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114105/new/
https://reviews.llvm.org/D114105
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
===================================================================
--- /dev/null
+++ 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));
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -58,6 +58,14 @@
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 @@
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 @@
IgnoreConversionFromTypes.empty()
? castExpr()
: castExpr(unless(hasSourceExpression(
- IsIgnoredTypeTwoLevelsDeep))))
+ IsIgnoredTypeTwoLevelsDeep))),
+ unless(ImplicitIntWidenedBitfieldValue))
.bind("cast")),
this);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
