[PATCH] D40854: [clang-tidy] WIP implement cppcoreguidelines check for mixed integer arithmetic

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> The "enforcement" listed on the C++ Core Guidelines is very unhelpful, so it might be worth bringing it up as an issue on their bug tracker. ES.100 basically says "you know what we mean, wink wink" as enforcement and doesn't give any guidance as to what is safe or unsafe. It gives no exceptions, which I think is unlikely to be useful to most developers. For instance: `void f(unsigned i) { if (i > 0) {} }`, this is a mixed signed and unsigned comparison (literals are signed by default) -- should this check diagnose?

I think the guidelines mostly aim for actual calculations here. For ES.102 there is an exception to "we want actual modulo arithmetic". They  seem mostly concerned with the mixing of the signedness of integer types that comes from explicitly expressing non-negative values with `unsigned` types. Because this is a common pattern (e.g. STL containers), the mixing from literals and co is quickly overseen.
See `ES.106: Don’t try to avoid negative values by using unsigned`

> How about `unsigned int i = 12; i += 1;`? ES.102 is equally as strange with "Flag unsigned literals (e.g. -2) used as container subscripts." That's a signed literal (2) with a negation operator -- do they mean "Flag container subscripts whose value is known to be negative", or something else?

I think here ES.106 is again the rationale but your example shows a hole. `unsigned int i = -1` is explicitly forbidden and the example shows a chain of implicit conversion of integer types as bad code.

My gut feeling is that the guidelines want programers to write `auto i = 12u` or `unsigned int = 12u` but they are not clear about that. Other rules that could relate to this are:
`ES.11: Use auto to avoid redundant repetition of type names`, `ES.23: Prefer the {} initializer syntax` (forbidding implicit conversions in initialization), `ES.48: Avoid casts` (maybe, but implicit conversion are not covered there).

I will ask them about this issue and hopefully we have some clarification. But I didn't have much success before :D

@aaron.ballman You are a maintainer for the `cert` rules, are you? How do you handle these common issues with integer types? Maybe we could already propose some guidance based on `cert`. It is mentioned as well written standard in the guidelines :)



================
Comment at: clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp:31
+      binaryOperator(allOf(anyOf(hasOperatorName("+"), hasOperatorName("-"),
+                                 hasOperatorName("*"), hasOperatorName("/")),
+                           hasEitherOperand(UnsignedIntegerOperand),
----------------
aaron.ballman wrote:
> What about `%` or comparisons operators?
I forgot `%`. Comparing `signed` and `unsigned` should be covered by front-end warnings (-Wsign-compare)


================
Comment at: test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp:167-168
+void pure_signed() {
+  int SInt1 = 42u;
+  signed char SChar1 = 42u;
+  SignedEnum SE1 = SEnum1;
----------------
aaron.ballman wrote:
> I think these are intended to be flagged under ES.102. "Flag results of unsigned arithmetic assigned to or printed as signed."
Yes they are but that was unintended :)

I should add a new section for such cases and include some logic in the matchers for it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40854