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

[Hal Finkel via cfe-dev]

----- Original Message -----

> From: "Richard Trieu" <rtrieu at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "cfe-dev" <cfe-dev at 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?

> On Fri, Apr 22, 2016 at 3:57 PM, Hal Finkel < hfinkel at anl.gov >
> wrote:

> > ----- Original Message -----
> 

> > > From: "Richard Trieu via cfe-dev" < cfe-dev at lists.llvm.org >
> 
> > > To: "cfe-dev" < cfe-dev at 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 (
> 
> > > https://webkit.org/code-style-guidelines/#null-false-and-zero )
> 
> > >
> 

> > 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. ;) 

> 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 

> > >
> 
> > > 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 (?:)
> 
> > > 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.
> 
> > > _______________________________________________
> 
> > > cfe-dev mailing list
> 
> > > cfe-dev at lists.llvm.org
> 
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> 
> > >
> 

> > --
> 
> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160422/2e5aed97/attachment.html>