This check implements the rule 5.6. HIC++
that forbidds bitwise operations on signed integer types.
Details
Diff Detail
- Build Status
Buildable 9772 Build 9772: arc lint + arc unit
Event Timeline
docs/clang-tidy/checks/hicpp-signed-bitwise.rst | ||
---|---|---|
7 | Please add short description. Copying it from ReleaseNotes.rst should be fine. |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | 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++. | |
43 | Any particular reason this local variable is needed? If so, it should not be declared with auto. | |
49 | s/and/an However, I think this assert can be removed as it cannot trigger unless the AST matching facilities have broken (which will break a *lot* of tests). | |
51 | Elide braces here and below. | |
53–57 | I think these diagnostics should be combined into a single diagnostic using a %select. Also, you can use getSourceRange() instead of manually creating the range. The wording of the diagnostic is a bit off, I would go with something more along the lines of: "use of a signed integer operand with a %select{binary|unary}0 bitwise operator" | |
58 | I'm not certain there is utility in highlighting the operator, since the operand is what's at fault. | |
63 | I don't think a note is appropriate here; I would include the SourceRange for the offending operand in the preceding diagnostic. |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | 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. | |
43 | This shortens the next 3 lines, otherwise there will be linebreaks that are bad for readability (IMHO). |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | 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. | |
48 | You can elide the parens, or use !!SignedBinary. | |
52 | The logic with IsBinary seems incorrect to me -- if its value is true, then it selects the "unary" option. |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
48 | Rather than go with the negative logic, why not go with IsUnary = SignedUnary != nullptr? |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | Yeah i see, i have such cases added in the tests. 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: 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". |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 |
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.
We already have such a warning in clang (-Wsign-conversion).
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. |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | 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) |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 | The response from Christof:
Your requested case is therefore conforming and i think the current implementation handles these things correct. |
clang-tidy/hicpp/SignedBitwiseCheck.cpp | ||
---|---|---|
24 |
Agreed; thank you for following up on this! | |
test/clang-tidy/hicpp-signed-bitwise.cpp | ||
206 | 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 }; |
test/clang-tidy/hicpp-signed-bitwise.cpp | ||
---|---|---|
206 | the first two cases int i1 ... are in the first function of this file( binary_bitwise). |
Is ignoring implicit casts the correct behavior here as far as the coding standard is concerned? Consider:
Looking at the coding standard, I get the impression this is not allowed, but I'm not *super* familiar with HIC++.