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