steakhal updated this revision to Diff 388240.
steakhal marked 2 inline comments as done.
steakhal edited the summary of this revision.
steakhal added a comment.
- added more tests
- add a detailed comment about the situation we suppress
- see through parenExprs
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,169 @@
+// 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;
+};
+
+void test_binary_and(SmallBitfield x) {
+ // CHECK-MESSAGES: [[@LINE+1]]:36: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ static_assert(is_same_v<decltype(x.id & 1), int>);
+ static_assert(is_same_v<decltype(x.id & 1u), unsigned>); // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id & 1;
+ x.id & 1u; // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 & x.id;
+ 1u & x.id; // no-warning
+}
+
+void test_binary_or(SmallBitfield x) {
+ // CHECK-MESSAGES: [[@LINE+1]]:36: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ static_assert(is_same_v<decltype(x.id | 1), int>);
+ static_assert(is_same_v<decltype(x.id | 1u), unsigned>); // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id | 1;
+ x.id | 1u; // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 | x.id;
+ 1u | x.id; // no-warning
+}
+
+void test(NoBitfield x) {
+ // no-warning in this function, except for operator+.
+ 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) {
+ // no-warning in this function: bitfield reads should be ignored within shift operations
+
+ // 'int' is large enough to hold the value of 'x.id', since that could at
+ // most populate 4 bits and an 'int' has 32.
+ 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;
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id + 1; // warn
+ x.id + 1u; // no-warning: promotion
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 + x.id; // warn
+ 1u + x.id; // no-warning: promotion
+}
+
+void test(BigBitfield x) {
+ // FIXME: We shall decide if we want a warning about this.
+
+ 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;
+
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id + 1; // warn
+ x.id + 1u; // no-warning: promotion
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 + x.id; // warn
+ 1u + x.id; // no-warning: promotion
+}
+
+void test(CompleteBitfield x) {
+ // no-warning in this function: there is no int -> unsigned conversion here
+
+ 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; // no-warning: x.id is 'unsigned int'
+ x.id + 1u; // no-warning: both operands are unsigned
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+
+ 1 + x.id; // no-warning: x.id is 'unsigned int'
+ 1u + x.id; // no-warning: both operands are unsigned
+}
+
+void test_parens(SmallBitfield x) {
+ // no-warning in this function: bitfield reads should be ignored within shift operations
+
+ 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
@@ -83,6 +83,48 @@
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 ShiftingIntWidenedBitfieldValue = castExpr(
+ hasCastKind(CK_IntegralCast), hasType(asString("int")),
+ has(castExpr(hasCastKind(CK_LValueToRValue),
+ has(ignoringParens(
+ memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+ hasParent(
+ binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
+
// Casts:
// i = 0.5;
// void f(int); f(0.5);
@@ -100,7 +142,8 @@
IgnoreConversionFromTypes.empty()
? castExpr()
: castExpr(unless(hasSourceExpression(
- IsIgnoredTypeTwoLevelsDeep))))
+ IsIgnoredTypeTwoLevelsDeep))),
+ unless(ShiftingIntWidenedBitfieldValue))
.bind("cast")),
this);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
