[PATCH] D114105: [clang-tidy] Ignore narrowing conversions in case of bitfields

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Thanks for the valuable feedback.

> D114105#inline-1089282 <https://reviews.llvm.org/D114105#inline-1089282>

Let me clarify my motivation, and why I'm interested in bitfields, conversions, and shift expressions.
Bitfields can be quite tricky.
It turns out doing anything useful with a bitfield will eventually result in an integral promotion, which mandates that the type must be 'int' if it fits, otherwise 'unsigned int' or no integral promotion happens otherwise.
conv.prom/5: <https://eel.is/c++draft/conv.prom#5>

> A prvalue for an integral bit-field ([class.bit]) can be converted to a prvalue of type **int if int can represent all the values of the bit-field**; otherwise, it can be converted to unsigned int if unsigned int can represent all the values of the bit-field. If the bit-field is larger yet, no integral promotion applies to it. If the bit-field has an enumerated type, it is treated as any other value of that type for promotion purposes.

This behavior really surprises developers, but the context in which it happens is mostly binary operators.
At first glance, those reports seem to be false-positives but they are actually true-positives - and we are happy catching them.
A recommended workaround to this problem could be simply supplying the proper integer literal constant, by adding the `u` unsigned-suffix.
This will turn the `int` operand into `unsigned int`, at which point the binary operator will suddenly do the right thing and promote the bitfield expression to `unsigned int` as well, and the warning disappears.
This //workaround// works just fine for the regular binary operators, whose result type is the promoted type of the operands.

In the expr.compound <https://eel.is/c++draft/expr.compound> section, the shift expression is the only one whose return type depends only on the //left// operand, making my plotted //workaround// inapplicable for them.
Note that the //compound shift assignment operator// expression doesn't suffer from this behavior.
expr.ass/1: <https://eel.is/c++draft/expr.compound#expr.ass-1>

> [...] The result in all cases is a bit-field if the left operand is a bit-field.

Given that the compiler chose `int` because that would be able to hold the value of the underlying bitfield without information loss, I think it makes sense to ignore these cases.
Of course, the shift operation might cause issues, but that's I think a different topic.
We might be able to do the extra mile and check if the other operand is a constant expression and the represented value would cause UB or unspecified behavior at the shift operation, but that's not really a conversion issue, what this check supposed to look for.

  x.id & 4u; // no-warning: promotion will force x.id to be unsigned
  x.id << 4u; // we still have a warning! the type of the LHS is unaffected by the type of the RHS
  (unsigned)x.id << 4; // looks weird, but does the right thing

That being said, I think the matcher should look through paren expressions but other than that I don't think there is an issue.
Alternatively, we could say that the users *must* use an explicit cast when loading from a bitfield in bitshifts to make their intention clear.
However, it doesn't feel natural and I cannot immediately see how frequently those reports would be true-positives.

WDYT? How should I proceed?



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:105
+                   has(memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+      hasParent(
+          binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
----------------
courbet wrote:
> What is specific about shifting operators ? What about `x+1` for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.
> 
> ```
> BinaryOperator <line:7:3, col:10> 'int' '+'
>       |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
>       | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
>       |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
>       |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
>       `-IntegerLiteral <col:10> 'int' 1
> ```
> What is specific about shifting operators ? What about `x+1` for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.
> 
> ```
> BinaryOperator <line:7:3, col:10> 'int' '+'
>       |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
>       | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
>       |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
>       |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
>       `-IntegerLiteral <col:10> 'int' 1
> ```




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114105/new/

https://reviews.llvm.org/D114105