Which floating point to bool conversions should trigger warnings?

I recently added (r267054) and then revert (r267234) some warnings for floating point to bool conversions pending more discussion on the topic. From my experience, bool conversions are tricky and a source of many bugs, so a warning in this place would be good.

Bool behaves a little differently than other integer types. For instance, 0.5 is converted to zero for integer types, except for bool which it gets converted to true. Also, bool is the type for conditionals and resulting type of logical operators.

However, at least one style guide says to use floating point to bool conversion (https://webkit.org/code-style-guidelines/#null-false-and-zero)

With that, I hope to get a little more discussion on this topic before implementing the floating point to bool conversion warnings again. The two factors for the warning are the source of the floating point value and where the conversion happens, although there may be other factors we need to take into account. Only the source has been taken account so far since it is the easier way to implement.

Source:

Exact floating point literals (0.0, 1.0)

Rounded floating point literals (0.5, 4.0)
Exact compiler time constant (5.0 - 4.0, kZero)
Inexact compiler time constant (1.0 / 2.0, kHugeNumber)
Run time values (getFloat())

Location:
Function Argument
Assigning/Initializing bool variable
Return statement of bool returning function
Condition of if statement, for loop, while loop, or do-while loop
Condition of conditional operator (?:slight_smile:
Operand of logical operators(&&, ||, !)

Currently, exact floating point literals are not warned on, rounded floating point literals are under -Wliteral-conversion, and everything else is under -Wfloat-conversion.

Note that exclusionary groups may also be useful, for instance “-Wfloat-conversion -Wno-some-bool-warning” if that helps filter out some of the more noisy warnings.

From: "Richard Trieu via cfe-dev" <cfe-dev@lists.llvm.org>
To: "cfe-dev" <cfe-dev@lists.llvm.org>
Sent: Friday, April 22, 2016 5:47:59 PM
Subject: [cfe-dev] Which floating point to bool conversions should trigger warnings?

I recently added (r267054) and then revert (r267234) some warnings
for floating point to bool conversions pending more discussion on
the topic. From my experience, bool conversions are tricky and a
source of many bugs, so a warning in this place would be good.

Bool behaves a little differently than other integer types. For
instance, 0.5 is converted to zero for integer types, except for
bool which it gets converted to true. Also, bool is the type for
conditionals and resulting type of logical operators.

However, at least one style guide says to use floating point to bool
conversion (
Code Style Guidelines | WebKit )

I don't see where it says that.

Regardless, I think we should always warn.

-Hal

> From: "Richard Trieu via cfe-dev" <cfe-dev@lists.llvm.org>
> To: "cfe-dev" <cfe-dev@lists.llvm.org>
> Sent: Friday, April 22, 2016 5:47:59 PM
> Subject: [cfe-dev] Which floating point to bool conversions should
trigger warnings?
>
> I recently added (r267054) and then revert (r267234) some warnings
> for floating point to bool conversions pending more discussion on
> the topic. From my experience, bool conversions are tricky and a
> source of many bugs, so a warning in this place would be good.
>
>
> Bool behaves a little differently than other integer types. For
> instance, 0.5 is converted to zero for integer types, except for
> bool which it gets converted to true. Also, bool is the type for
> conditionals and resulting type of logical operators.
>
>
> However, at least one style guide says to use floating point to bool
> conversion (
> Code Style Guidelines | WebKit )
>

I don't see where it says that.

Regardless, I think we should always warn.

-Hal

It specifies that checks against 0 should be written without the == 0. For
example:

  void check(float f) {
    if (f) { } // This is the same as "f != 0.0"
    if (!f) { } // This is the same as "f == 0.0"
  }

I don't intend to stop warning, just split the warning into pieces so users
can better control which warnings are active.

From: "Richard Trieu" <rtrieu@google.com>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: "cfe-dev" <cfe-dev@lists.llvm.org>
Sent: Friday, April 22, 2016 6:04:52 PM
Subject: Re: [cfe-dev] Which floating point to bool conversions
should trigger warnings?

> > From: "Richard Trieu via cfe-dev" < cfe-dev@lists.llvm.org >

> > To: "cfe-dev" < cfe-dev@lists.llvm.org >

> > Sent: Friday, April 22, 2016 5:47:59 PM

> > Subject: [cfe-dev] Which floating point to bool conversions
> > should
> > trigger warnings?

> >

> > I recently added (r267054) and then revert (r267234) some
> > warnings

> > for floating point to bool conversions pending more discussion on

> > the topic. From my experience, bool conversions are tricky and a

> > source of many bugs, so a warning in this place would be good.

> >

> >

> > Bool behaves a little differently than other integer types. For

> > instance, 0.5 is converted to zero for integer types, except for

> > bool which it gets converted to true. Also, bool is the type for

> > conditionals and resulting type of logical operators.

> >

> >

> > However, at least one style guide says to use floating point to
> > bool

> > conversion (

> > Code Style Guidelines | WebKit )

> >

> I don't see where it says that.

> Regardless, I think we should always warn.

> -Hal

It specifies that checks against 0 should be written without the ==
0. For example:

void check(float f) {
if (f) { } // This is the same as "f != 0.0"
if (!f) { } // This is the same as "f == 0.0"

}

Checks against 0 are different from checks against 0.0. Your example does not appear in the webkit document, and I hope it never does. :wink:

I don't intend to stop warning, just split the warning into pieces so
users can better control which warnings are active.

I've seen code that uses float-to-bool conversions, for example, in cases where a parameter is never really zero unless some entire feature is disabled. Thus, they test the feature's enabled state with a float-to-bool conversion on the parameter value. Perhaps, to support a subset of this use case, one might want to warn on all cases except for exactly 0.0. How much value this adds, however, is unclear because in many cases these are runtime checks.

In any case, unless you have some particular use case for subsetting the feature, I recommend warning everywhere until a concrete use case is presented for doing otherwise.

-Hal