[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+      binaryOperator(
+          anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+          hasLHS(hasType(isInteger())),
----------------
aaron.ballman wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > Why only these two operators? This does not match the behavior from the C++ Core Guideline check itself (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing).
> > These provided the best signal to noise ratio. Also they are the most dangerous (in a loop, you might end up losing one unit per iteration). I'll add other operators later if that's fine with you.
> If the check doesn't cover anything listed in the C++ Core Guideline examples or enforcement wording, I have a hard time accepting it as the initial commit for such a check.
> 
> Of special interest to me is the request from the C++ Core Guideline authors themselves: 
> ```
>   - flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data)
>   - flag all long->char (I suspect int->char is very common. Here be dragons! we need data)
> ```
> I think we need data as well -- writing the check and then testing it over several million lines of source could give us some of that data, which we can then feed back to the guideline authors, so that we come up with a check that's realistic.
Quoting the guideline:
"A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives." Then follows a list of suggestions. While +=/-= are not in the list of suggestions, they are a subset of "flag all floating-point to integer conversions" that I've found to be useful (very low rate of false positives) by running on our codebase.
I agree that more data would be useful, so I'll do an analysis of flagging all (non-ceil/floor float/double)->integral conversions.






Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455