[PATCH] D36586: [clang-tidy] hicpp bitwise operations on signed integers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/hicpp/SignedBitwiseCheck.cpp:23
+  const auto SignedIntegerOperand =
+      expr(ignoringImpCasts(hasType(isSignedInteger()))).bind("signed_operand");
+
----------------
JonasToth wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > aaron.ballman wrote:
> > > > > JonasToth wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Is ignoring implicit casts the correct behavior here as far as the coding standard is concerned? Consider:
> > > > > > > ```
> > > > > > > unsigned char c = 128;
> > > > > > > f(~c); // c promotes to int before ~ is applied
> > > > > > > ```
> > > > > > > Looking at the coding standard, I get the impression this is not allowed, but I'm not *super* familiar with HIC++.
> > > > > > I first implemented it without ignoring the implicit integer casts, the result was, that most cases (in test cases) where not found. therefore i implemented it that way. I add an testcase for this and see how i need to adjust the matcher.
> > > > > > 
> > > > > > Could you help me there with the semantic, since i am not so fluent in C/C++ standardese, but i think the findings are reasonable.
> > > > > It kind of boils down to the intention from the HIC++. Consider a test case like:
> > > > > ```
> > > > > void f(int i);
> > > > > 
> > > > > void g() {
> > > > >   unsigned char c = 127;
> > > > >   f(~c);
> > > > > }
> > > > > 
> > > > > ```
> > > > > Does `f()` expect to receive `-128` or `128`? I think this code will pass your check (ignoring the promotion means the type is `unsigned char`), but the actual bitwise operation is on a signed integer value because there is an integer promotion. So 127 is promoted to int, then ~ is applied, resulting in the value `-128` being passed to the function.
> > > > Yeah i see, i have such cases added in the tests.
> > > > TBH. i don't know if the standard wants this covered, but the demonstrated case is definitly bad.
> > > > 
> > > > Would it be a good idea, to warn on assigning/initializing `signed` integers with `unsigned` integers?
> > > > 
> > > > The CppCoreGuidelines have some sections on that as well: [[ https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#arithmetic | Section ES.100-103 ]]
> > > > 
> > > > Not sure if this check should care. On the other hand, would it  be nice to have a check that covers all "integer problems".
> > > > Yeah i see, i have such cases added in the tests.
> > > > TBH. i don't know if the standard wants this covered, but the demonstrated case is definitly bad.
> > > 
> > > I think you should ask the HIC++ people what they think; the rule text does not make it clear what the behavior should be here.
> > > 
> > > > Would it be a good idea, to warn on assigning/initializing signed integers with unsigned integers?
> > > 
> > > We already have such a warning in clang (-Wsign-conversion).
> > > 
> > > >The CppCoreGuidelines have some sections on that as well: Section ES.100-103
> > > >
> > > >Not sure if this check should care. On the other hand, would it be nice to have a check that covers all "integer problems".
> > > 
> > > Any such check will require so many options that it is likely to be almost unusable. However, I would not be opposed to seeing a clang-tidy module that has a bunch of checks in it that relate to integer problems.
> > i think, those could land in `bugprone-`, and be aliases to hicpp and the cppcoreguidelines if appropriate.
> > 
> > depending on my time (i should have a lot of free time for 2 months) 
> > i will try to implement some.
> > is there a resource with all common problems found with integers?
> The response from Christof:
> 
> 
> 
> > Hi Jonas,
> > 
> > using bitwise operators with unsigned operands is compliant with this
> > rule, even if the operand is promoted to signed int.
> > 
> > expr.unary.op in the C++ standard says "The operand of ~ shall have
> > integral or unscoped enumeration type; [...] Integral promotions are
> > performed. The type of the result is the type of the promoted operand."
> > 
> > The HICPP rule refers to the type of the operand (and not the type of
> > the promoted operand), it therefore refers to the type before promotion
> > (which is "unsigned char" in the example).
> > 
> > 
> > Regards,
> > Christof
> 
> Your requested case is therefore conforming and i think the current implementation handles these things correct.
> Your requested case is therefore conforming and i think the current implementation handles these things correct.

Agreed; thank you for following up on this!


================
Comment at: test/clang-tidy/hicpp-signed-bitwise.cpp:205
+
+#if 0
+// Scoped Enums must define their operations, so the overloaded operators must take care
----------------
I don't think it helps to have this code remain in the test case.

Can you add tests for the following?
```
int i1 = 1 << 12;
int i2 = 1u << 12;

enum E {
  one = 1,
  two = 2,
  test1 = 1 << 12,
  test2 = one << two,
  test3 = 1u << 12
};
```


https://reviews.llvm.org/D36586