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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
