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

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:104
+          castExpr(hasCastKind(CK_LValueToRValue),
+                   has(memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+      hasParent(
----------------
There needs to be some kind of check of the size of the bit field vs the size of the integer, because `struct SmallBitfield { unsigned int id : 31; };` should warn.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:105
+                   has(memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+      hasParent(
+          binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
----------------
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
```


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:106
+      hasParent(
+          binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
+
----------------
There also needs to be some check of the constant value of the RHS, because `x.id << 30` can actually overflow, so we want to warn in that case.


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

https://reviews.llvm.org/D114105