Bug 106774

Summary: warning about comparison to true/false
Product: gcc Reporter: Frank Heckenbach <f.heckenbach>
Component: c++Assignee: Not yet assigned to anyone <unassigned>
Status: UNCONFIRMED ---    
Severity: enhancement CC: egallager, marxin, webrown.cpp
Priority: P3 Keywords: diagnostic
Version: unknown   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed:
Bug Depends on:    
Bug Blocks: 87403    

Description Frank Heckenbach 2022-08-29 23:48:19 UTC 
gcc has "-Wbool-compare" to warn about boolean expression compared with an integer value different from true/false. This warning is enabled by -Wall.

However, a warning for comparisons with true and false may also be useful. The former is redundant, the latter can be replaced with "!".

Actually, this warning may also be given when comparing a non-Boolean integral expression with true or false. The former is not redundant, but may often not be what's intended (if so, one should compare with 1). The latter is equivalent to comparing with 0, but still strange style. And comparing non-integral (e.g. floating-point) expressions with Boolean literals is certainly warning-worthy (though it may be caught by "-Wfloat-equal" as well).

To sum up, a new warning about comparing any expression with true/false literals. 

Maybe something like "-Wbool-compare=2" (which would not be enabled by -Wall, I guess; maybe by -Wextra, but even if it must be given manually, I'd find it useful).
Comment 1 Andrew Pinski 2022-08-29 23:52:24 UTC 
Do you have an example?
Comment 2 Frank Heckenbach 2022-08-30 00:02:22 UTC 
This should cover all cases mentioned:

void t (bool a, int i, float e, double f)
{
  // Boolean literal comparisons
  if (a == true)  // better: if (a)
    return;
  if (a == false)  // better: if (!a)
    return;
  if (a != true)  // better: if (!a)
    return;
  if (a != false)  // better: if (a)
    return;
  // also reversed to be sure
  if (true == a)  // better: if (a)
    return;
  if (false != a)  // better: if (a)
    return;

  // Integer comparisons to Boolean literals
  if (i == true)  // better: if (i == 1)
    return;
  if (i == false)  // better: if (i == 0) or: if (!i)
    return;  
  if (i != true)  // better: if (i != 1)
    return;
  if (i != false)  // better: if (i != 0) or: if (i)
    return;

  // Floating-point comparisons to Boolean literals
  if (e == true)  // very strange at all, if meant so, then better: if (e == 1.0f)
    return;
  if (f == false)  // very strange at all, if meant so, then better: if (f == 0.0)
    return;
}
Comment 3 Richard Biener 2022-08-30 07:09:41 UTC 
that's more coding style though?
Comment 4 Eric Gallager 2022-08-30 23:50:00 UTC 
(In reply to Richard Biener from comment #3)
> that's more coding style though?

Yeah I personally prefer the more explicit way of writing it with both operands myself
Comment 5 Jonathan Wakely 2022-08-31 10:33:44 UTC 
(In reply to Frank Heckenbach from comment #2)
> This should cover all cases mentioned:
> 
> void t (bool a, int i, float e, double f)
> {
>   // Boolean literal comparisons
>   if (a == true)  // better: if (a)
>     return;
>   if (a == false)  // better: if (!a)
>     return;
>   if (a != true)  // better: if (!a)
>     return;
>   if (a != false)  // better: if (a)
>     return;
>   // also reversed to be sure
>   if (true == a)  // better: if (a)
>     return;
>   if (false != a)  // better: if (a)
>     return;

These are definitely coding style, and I don't think a warning is warranted.

> 
>   // Integer comparisons to Boolean literals
>   if (i == true)  // better: if (i == 1)

Or maybe it was meant to be ((bool)i = true).

Warning for these does seem more useful IMO. The arithmetic promotions from bool to int (or float) mean the comparison doesn't do what it appears to do, and there seems no reason to use a true/false literal there.
Comment 6 Frank Heckenbach 2022-08-31 23:46:44 UTC 
> --- Comment #4 from Eric Gallager <egallager at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #3)
> > that's more coding style though?
> 
> Yeah I personally prefer the more explicit way of writing it with both operands
> myself

It's not really about operands, but the whole operation is
redundant: "mybool == true" is exactly equivalent to just "mybool".

So it's similar to a cast to the original type which GCC can also
warn about (-Wuseless-cast).

Some other existing warnings (-Wredundant-decls, -Wredundant-move)
also seem to warn about things that are no potential problems, just
redundant.

Of course, all of this can be considered a matter of style, that's
why warnings can be selected by options. (As I said, I assume it
would not be enabled by -Wall, and I don't mind whether or not it's
enabled by -Wextra.)