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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+      binaryOperator(
+          anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+          hasLHS(hasType(isInteger())),
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455