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

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

steakhal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:105
+                   has(memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+      hasParent(
+          binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
----------------
steakhal wrote:
> 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
> > ```
> 
> 
I'm not sure if this approach is the right approach.
In that sense, we should not act differently on shift operations, rather just ignore member access expressions implicitly promoted to `int`.
This is what we are actually looking for. I would rather pursue this direction, but it would mean that we suppress a larger set of reports that we currently have.

What's your opinion on this @courbet @aaron.ballman?


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

https://reviews.llvm.org/D114105