Page MenuHomePhabricator

[clang-tidy] hicpp bitwise operations on signed integers
ClosedPublic

Authored by JonasToth on Aug 10 2017, 12:07 PM.

Event Timeline

JonasToth created this revision.Aug 10 2017, 12:07 PM
Eugene.Zelenko added inline comments.Aug 10 2017, 1:01 PM
docs/clang-tidy/checks/hicpp-signed-bitwise.rst
7

Please add short description. Copying it from ReleaseNotes.rst should be fine.

JonasToth updated this revision to Diff 110628.Aug 10 2017, 1:27 PM
JonasToth marked an inline comment as done.

Fix documentation of checker

Eugene.Zelenko added inline comments.Aug 10 2017, 6:53 PM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
59

Unnecessary empty line.

docs/clang-tidy/checks/hicpp-signed-bitwise.rst
9

Please insert empty line above.

JonasToth marked 2 inline comments as done.Aug 11 2017, 3:18 AM
JonasToth updated this revision to Diff 110681.Aug 11 2017, 3:19 AM

fix review comments from eugene

aaron.ballman added inline comments.Aug 11 2017, 6:23 AM
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.

JonasToth marked 5 inline comments as done.Aug 11 2017, 7:27 AM
JonasToth added inline comments.
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).

JonasToth updated this revision to Diff 110721.Aug 11 2017, 7:27 AM
JonasToth marked an inline comment as done.

adress aarons comments

JonasToth marked an inline comment as done.Aug 11 2017, 7:34 AM
JonasToth updated this revision to Diff 110723.Aug 11 2017, 7:35 AM

adjusted diagnostics

aaron.ballman added inline comments.Aug 11 2017, 9:19 AM
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.

JonasToth marked 2 inline comments as done.

minor stuff and rebase

aaron.ballman added inline comments.Aug 11 2017, 12:54 PM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
48

Rather than go with the negative logic, why not go with IsUnary = SignedUnary != nullptr?

JonasToth added inline comments.Aug 11 2017, 1:03 PM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
24

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: 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".

JonasToth updated this revision to Diff 110796.Aug 11 2017, 1:17 PM

minor fix for logic, testcase improved

JonasToth marked an inline comment as done.Aug 11 2017, 1:18 PM
aaron.ballman added inline comments.Aug 16 2017, 6:42 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
24

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.

JonasToth added inline comments.Aug 24 2017, 8:41 AM
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)
i will try to implement some.
is there a resource with all common problems found with integers?

JonasToth updated this revision to Diff 112572.Aug 24 2017, 9:12 AM
  • get up to date with master
  • added testcase for enums
JonasToth updated this revision to Diff 112573.Aug 24 2017, 9:15 AM
  • fix indendation in testcase
JonasToth added inline comments.Aug 30 2017, 2:32 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
24

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.

aaron.ballman added inline comments.Aug 30 2017, 4:54 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
24

Your requested case is therefore conforming and i think the current implementation handles these things correct.

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
};
JonasToth updated this revision to Diff 113246.Aug 30 2017, 6:04 AM
JonasToth marked 8 inline comments as done.
  • added additional testcases, like @aaron.ballman requested
  • fixed diagnostics
JonasToth marked an inline comment as done.Aug 30 2017, 6:06 AM
JonasToth added inline comments.
test/clang-tidy/hicpp-signed-bitwise.cpp
206

the first two cases int i1 ... are in the first function of this file( binary_bitwise).

JonasToth marked 2 inline comments as done.Aug 30 2017, 6:06 AM
aaron.ballman accepted this revision.Aug 30 2017, 6:17 AM

Thanks! LGTM!

This revision is now accepted and ready to land.Aug 30 2017, 6:17 AM
JonasToth closed this revision.Aug 30 2017, 6:33 AM