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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D40854#950640, @JonasToth wrote:

> > 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


Thanks! As it stands, I'm not able to determine whether your implementation actually does or does not enforce those rules. That's why I was wondering what the extent of the coverage is according to the rule authors, because I am not certain whether we're actually implementing the intent of the rules or not.

> @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 :)

Yes, I used to maintain the CERT rules when I worked for the SEI (and I authored a considerable number of them). CERT and the C++ Core Guidelines have a somewhat fundamental difference in opinion on how we write the rules. CERT flags things that are demonstrably going to lead to incorrectness (specifically, vulnerabilities). The C++ Core Guidelines flag things that could possibly lead to correctness issues but may also be perfectly correct code. Because of this difference, CERT rules are a bit harder to implement in clang-tidy because they often will have some component of data or flow analysis to them, while C++ Core Guidelines are often blanket bans.

That said, CERT's guidance on integers mostly falls back to the C rules: https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152052

I would recommend looking at the C++ Core Guideline wording and try to come up with example code that would run afoul of the rule to see if *you* think a diagnostic would be useful with that code. If you can't convince yourself that diagnosing the code is a good idea, it's worth bringing up to the authors to see if they agree. Similarly, see if you can find example code that would not run afoul of the rules but you think *should* to see if they agree. (You can use the CERT rules on integers to help give ideas.) Once you have a corpus of examples you want to see diagnosed and not diagnosed, try to devise (possibly new) wording for the enforcement section of the rule and see if the rule authors agree with the enforcement. From there, writing the check becomes a pretty simple exercise in translating the enforcement into a clang-tidy check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40854