JonasToth added inline comments.
================
Comment at: clang-tidy/hicpp/SignedBitwiseCheck.cpp:23
+ const auto SignedIntegerOperand =
+
expr(ignoringImpCasts(hasType(isSignedInteger()))).bind("signed_operand");
+
----------------
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?
https://reviews.llvm.org/D36586
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
