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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits