[cfe-commits] [PATCH] Warning for suspicious implicit conversions from floating point to bool

[David Blaikie]

On Thu, Aug 16, 2012 at 8:23 AM, Andreas Eckleder <aeckleder at google.com> wrote:
> Hello,
>
> A while ago, I posted a patch introducing a warning for suspicious
> implicit conversions from floating point to bool.

Hmm sorry - I must've missed that. I proposed something slightly more
general a few months ago & had trouble getting the false positive rate
down to an acceptable level.

I'm curious though: What false positives did you see for float->bool
without the extra argument/context based filtering you've implemented?

> Since then, I've run
> quite an extensive analysis of true and false positives this warning
> produces when applied to Google's code base.
>
> While the initial warning produced a low 4 digits number of hits with
> only about 10%-20% of them being sufficiently "suspicious" to merit
> further investigation, the new patch limits the warning two two
> specific cases with a 100% true positive rate:
>
> 1) Unintended swapping of function call arguments
>
> This catches situations where e.g. as a result of a method signature
> change parameters are specified in the wrong order, e.g.
>
> void foo(bool b, float f);
> foo(1.7, false);
>
> The patch will yield a warning if there is an implicit floating point
> to bool conversion for a function call argument with another implicit
> conversion for one of the neighboring arguments.
>
> This produced a low 2-digits number of hits in our code base, all of
> which are true positives.
>
> 2) Misplaced brackets around function call arguments
>
> This catches situations where brackets are placed in a complex
> expression, the typical example of which I found to be something like
> bool InRange = fabs( a - b < delta );
>
> To catch these instances, the patch will produce a warning for an
> implicit floating point to bool conversion of a function call result
> if the last function call argument happens to be converted implicitly
> from bool to float.
>
> This produced a high 2-digits number of hits in out code base, all of
> which are true positives.
>
> I'm sure the patch does not yet catch all problematic instances of
> floating point to bool conversions, so more cases could be added to
> this in the future. Also, it might be interesting to evaluate whether
> the two patterns above can also be observed for other implicit type
> conversions, although I expect the floating point to bool case to be
> the most frequent one, especially for case 2).
>
> As before, the diagnostic is can be turned on using
> -Wimplicit-conversion-floating-point-to-bool (off by-default).
> The patch contains a test case
> (test/SemaCXX/warn-implicit-conversion-floating-point-to-bool.cpp).
>
> Example:
> compile this code with -Wimplicit-conversion-floating-point-to-bool
>
> float foof(float x) {
>   return x;
> }
>
> double food(double x) {
>   return x;
> }
>
> void foo(bool b, float f) { }
>
> void bar() {
>
>   float c = 1.7;
>   bool b = c; // no warning
>
>   double e = 1.7;
>   b = e; // no warning
>
>   b = foof(4.0); // no warning
>
>   b = foof(c<1); // expected-warning {{implicit conversion turns
> floating-point number into bool: 'float' to 'bool'}}
>
>   b = food(e<2); // expected-warning {{implicit conversion turns
> floating-point number into bool: 'double' to 'bool'}}
>
>   foo(c, b);    // expected-warning {{implicit conversion turns
> floating-point number into bool: 'float' to 'bool'}}
>   foo(c, c); // no warning
>
> }
>
> Please, review this patch.
> Thanks!
>
> Andreas
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>