[cfe-commits] [Patch] Add new warning group and warnings for questionable boolean compares

[Eli Friedman]

On Mon, Oct 29, 2012 at 2:26 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Oct 29, 2012 at 2:01 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Mon, Oct 29, 2012 at 1:48 PM, Richard Trieu <rtrieu at google.com> wrote:
>>> On Mon, Oct 29, 2012 at 1:43 PM, Eli Friedman <eli.friedman at gmail.com>
>>> wrote:
>>>>
>>>> On Mon, Oct 29, 2012 at 1:28 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>> > On Fri, Oct 26, 2012 at 5:37 PM, Eli Friedman <eli.friedman at gmail.com>
>>>> > wrote:
>>>> >>
>>>> >> On Fri, Oct 26, 2012 at 5:20 PM, Richard Trieu <rtrieu at google.com>
>>>> >> wrote:
>>>> >> > -Wbool-compare-tautological
>>>> >> > Comparisons involving a boolean and an expression evaluating to 1, 0,
>>>> >> > true,
>>>> >> > or false such that the comparison always evaluate to true or false.
>>>> >> > Also added to -Wtautological-compare group
>>>> >> > High true positive rate.
>>>> >> > Not previously caught by -Wtautological-compare
>>>> >>
>>>> >> Please just fix DiagnoseOutOfRangeComparison to do the right thing.
>>>> >>
>>>> >> > -Wbool-compare-tautological-out-of-range
>>>> >> > Comparisons of a boolean and >1 or negative values.
>>>> >> > Also added to -Wtautological-constant-out-of-range-compare group
>>>> >> > High true positive rate.
>>>> >> > Previously, only compares with values >1 were caught.
>>>> >> > (x == 5)  currently caught
>>>> >> > (x == -1)  not currently caught
>>>> >>
>>>> >> Same.
>>>> >>
>>>> >> -Eli
>>>> >
>>>> >
>>>> > What do you think of the idea of separating out warnings on bool
>>>> > comparisons
>>>> > into a sub group?
>>>>
>>>> I'm not exactly opposed... but why?  The tautological compare warning
>>>> is on by default anyway.
>>>
>>> I think that finer control over warnings is a good thing.  And that this
>>> warning would fit in nicely if a new -Wbool-compare is created.
>>
>> We shouldn't add new warning groups just for the sake of having more
>> warning groups.  I think the value argument here is weak, but maybe
>> I'm missing something.
>
> We've had to turn off -Wtautological-compare due to the frustratingly
> high rate of occurrence, and too high of a false-positive rate for us
> to clean up all the code for. However, some of the fine grained pieces
> Richard is teasing out here will be significantly easier for us to
> roll out to our users, and will help incrementally attack the problem
> of the larger warning.

Okay, that's fine.  But it needed to be said.

-Eli