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' + // | `-ImplicitCastExpr 'unsigned int' + // | `-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); 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,202 @@ +// 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 +struct is_same { + static constexpr bool value = false; +}; +template +struct is_same { + static constexpr bool value = true; +}; + +template +static constexpr bool is_same_v = is_same::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); + static_assert(is_same_v); + + x.id & 1; + x.id & 1u; + + 1 & x.id; + 1u & x.id; +} + +void test_binary_or(SmallBitfield x) { + static_assert(is_same_v); + static_assert(is_same_v); + + x.id | 1; + x.id | 1u; + + 1 | x.id; + 1u | x.id; +} + +template +void take(T); +void test_parameter_passing(NoBitfield x) { + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined + take(x.id); + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined + take(x.id); + take(x.id); +} + +void test_parameter_passing(SmallBitfield x) { + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined + take(x.id); + take(x.id); // no-warning + take(x.id); + take(x.id); +} + +void test_parameter_passing(BigBitfield x) { + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined + take(x.id); + take(x.id); // no-warning + take(x.id); + take(x.id); +} + +void test_parameter_passing(CompleteBitfield x) { + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: narrowing conversion from 'unsigned int' to signed type 'char' is implementation-defined + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'unsigned int' to signed type 'short' is implementation-defined + take(x.id); + take(x.id); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined + take(x.id); + take(x.id); +} + +void test(NoBitfield x) { + static_assert(is_same_v); + static_assert(is_same_v); + static_assert(is_same_v); + static_assert(is_same_v); + + 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); + static_assert(is_same_v); + + 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); + static_assert(is_same_v); + + 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); + static_assert(is_same_v); + + 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); + static_assert(is_same_v); + x.id << (2); + ((x.id)) << (2); + + static_assert(is_same_v); + static_assert(is_same_v); + (2) << x.id; + (2) << ((x.id)); +}