[Bug preprocessor/66322] New: Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[Bug preprocessor/66322] New: Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[t.artem at mailcity dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

            Bug ID: 66322
           Summary: Linus Torvalds: -Wswitch-bool produces dubious
                    warnings, fails to notice really bad things
           Product: gcc
           Version: 5.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: preprocessor
          Assignee: unassigned at gcc dot gnu.org
          Reporter: t.artem at mailcity dot com
  Target Milestone: ---

From: https://lkml.org/lkml/2015/5/27/941

Btw, I'd actually like to see (possibly optionally) a warning for enum
types there too. Exactly because *type* based warnings very much make
sense, regardless of number of cases.

For example, try this:

    #include <stdbool.h>
    #include <stdio.h>

    enum a { one, two };
    int t(bool b, enum a e)
    {
        switch (b) {
        case true:
            printf("No arguments\n");
            /* fallthrough */
        case false:
            printf("\n");
        }
        switch (e) {
        case 0:
            printf("one");
            break;
        case two:
            printf("two");
            break;
        }
        return 0;
    }
and I'd argue that gcc-5.1 warns about TOTALLY THE WRONG THING.

It does that *stupid* warning:

    warning: switch condition has boolean value [-Wswitch-bool]

which is just idiotic and wrong.

The case statements are clearly boolean, there is absolutely nothing
wrong with that switch, and a compiler that warns about it is just
being f*cking moronic.

In contrast, that second switch() statement with the "case 0:" is
actually something that might well be worth warning for. I'd argue
that the code would clearly be more legible if it used "case one:"
instead.

So the new warning in gcc-5 seems to be just stupid. In general,
warnings that encourage you to write bad code are stupid. The above

    switch (boolean) {
    case true:
is *good* code, while the gcc documentation suggests that you should
cast it to "int" in order to avoid the warning, but anybody who
actually thinks that

    switch ((int)boolean) {
    switch 1:
is better, clearly has absolutely zero taste and is just objectively wrong.

Really. A warning where the very *documentation* tells you to do
stupid things is stupid.

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #1 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
I think originally, the warning was supposed to catch things like

switch (n && MASK) {}

or even using the wrong variable in the switch(). Of course, it could be
smarter and avoid warning about the specific case 'switch(bool) {case true:
case false:}', which is what Linus says here:
https://lkml.org/lkml/2015/5/27/933

If only GCC had 1/100th of the developers of the Linux kernel... 

The manual and the warning text could also say explicitly the reason for
warning (something like "often indicates a typo")
>From gcc-bugs-return-487479-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu May 28 08:29:24 2015
Return-Path: <gcc-bugs-return-487479-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 32081 invoked by alias); 28 May 2015 08:29:24 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 32042 invoked by uid 48); 28 May 2015 08:29:20 -0000
From: "jakub at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things
Date: Thu, 28 May 2015 08:29:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: c
X-Bugzilla-Version: 5.1.0
X-Bugzilla-Keywords: diagnostic
X-Bugzilla-Severity: normal
X-Bugzilla-Who: jakub at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: cc
Message-ID: <bug-66322-4-MVoHoqsUrH@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66322-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66322-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-05/txt/msg02319.txt.bz2
Content-length: 588

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf322

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Well, using switch on bool is always weird, one really should use if for that.
If you want fallthrough, then just use if (cond) { first } second, if without
fallthrough, then if (cond) { first } else { second }.

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Of course, I don't think

    switch ((int)boolean) {
    switch 1:

is better or nice, and I wouldn't want to encourage writing such a code, the
cast (you don't really have to change true to 1) is only meant as a way to
quash the warning.  But switch (boolean) seemed to be too obscure that giving a
warning there didn't feel too far-fetched; especially if you can use the cast
(in C, integer promotions are performed on the controlling expression anyway)
if you really want that.

To fix this, I think I'll have to defer the warning until c_finish_case and add
some flag to c_switch struct and track whether the controlling expression had
boolean type there.  Then in c_do_switch_warnings decide if we want to warn or
not.

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
I guess we also shouldn't warn on

(1) switch (bool)
    {
    case true: ...
    default: ...
    }

(2) switch (bool)
    {
    case true: ...
    }

(3) switch (bool)
    {
    default:
    }

Similarly with s/true/false/.

BTW, I've checked what clang does and the behavior is the same as GCC 5.1.

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[nszabolcs at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

Szabolcs Nagy <nszabolcs at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nszabolcs at gmail dot com

--- Comment #5 from Szabolcs Nagy <nszabolcs at gmail dot com> ---
i don't mind the warning (there are other warnings in gcc with false
positives), but the documentation must not encourage the use of casts for
dealing with it.

that is dangerous and much worse than the original problem: the cast turns off
the type-system so if the type of the expression later changes then anything
goes (pointers, floats, different int types).

so i think this is the real bug:
https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Warning-Options.html#index-Wswitch-bool-362

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

--- Comment #6 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> Well, using switch on bool is always weird, one really should use if for
> that.
> If you want fallthrough, then just use if (cond) { first } second, if without
> fallthrough, then if (cond) { first } else { second }.

Indeed. I wrote:

https://gcc.gnu.org/wiki/VerboseDiagnostics#warning:_switch_condition_has_boolean_value_.5B-Wswitch-bool.5D

for future reference. I think there will be always "valid" cases that GCC
cannot avoid warning, so perhaps it is useful to explain the reasoning behind
it and what is the best way to work-around it. (It seems the cast is after all
not the best way, converting it to if-else seems better).
>From gcc-bugs-return-487504-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu May 28 11:54:16 2015
Return-Path: <gcc-bugs-return-487504-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 59738 invoked by alias); 28 May 2015 11:54:14 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 59630 invoked by uid 55); 28 May 2015 11:54:09 -0000
From: "redi at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug libstdc++/65352] array<T,0>::begin()/end() etc. forms a null reference and breaks on clang+ubsan
Date: Thu, 28 May 2015 11:54:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: libstdc++
X-Bugzilla-Version: 4.9.2
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: redi at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-65352-4-lK6zKKTh8y@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-65352-4@http.gcc.gnu.org/bugzilla/>
References: <bug-65352-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-05/txt/msg02344.txt.bz2
Content-length: 875

https://gcc.gnu.org/bugzilla/show_bug.cgi?ide352

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Author: redi
Date: Thu May 28 11:53:35 2015
New Revision: 223806

URL: https://gcc.gnu.org/viewcvs?rev"3806&root=gcc&view=rev
Log:
        PR libstdc++/65352
        * include/std/array (__array_traits::_S_ptr): New function.
        (array::data): Use _S_ptr to avoid creating invalid reference.
        * testsuite/23_containers/array/tuple_interface/get_neg.cc: Adjust
        dg-error line numbers.
        * testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc:
        likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/array
    trunk/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc

trunk/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Patch posted some time ago:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00790.html

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

--- Comment #8 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Mon Jun 29 13:12:44 2015
New Revision: 225116

URL: https://gcc.gnu.org/viewcvs?rev=225116&root=gcc&view=rev
Log:
        PR c/66322
        * c-common.c (check_case_bounds): Add bool * parameter.  Set
        OUTSIDE_RANGE_P.
        (c_add_case_label): Add bool * parameter.  Pass it down to
        check_case_bounds.
        (c_do_switch_warnings): Add bool parameters.  Implement -Wswitch-bool
        warning here.
        * c-common.h (c_add_case_label, c_do_switch_warnings): Update
        declarations.

        * c-typeck.c (struct c_switch): Add BOOL_COND_P and OUTSIDE_RANGE_P.
        (c_start_case): Set BOOL_COND_P and OUTSIDE_RANGE_P.  Don't warn
        about -Wswitch-bool here.
        (do_case): Update c_add_case_label call.
        (c_finish_case): Update c_do_switch_warnings call.

        * decl.c (struct cp_switch): Add OUTSIDE_RANGE_P.
        (push_switch): Set OUTSIDE_RANGE_P.
        (pop_switch): Update c_do_switch_warnings call.
        (finish_case_label): Update c_add_case_label call.
        * semantics.c (finish_switch_cond): Don't warn about -Wswitch-bool
        here.

        * function.c (stack_protect_epilogue): Remove a cast to int.
        * doc/invoke.texi: Update -Wswitch-bool description.

        * c-c++-common/pr60439.c: Add dg-prune-output and add switch cases.
        * c-c++-common/pr66322.c: New test.
        * g++.dg/eh/scope1.C: Remove dg-warning.

Added:
    trunk/gcc/testsuite/c-c++-common/pr66322.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-typeck.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/semantics.c
    trunk/gcc/doc/invoke.texi
    trunk/gcc/function.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/pr60439.c
    trunk/gcc/testsuite/g++.dg/eh/scope1.C

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed for GCC 6.

[Bug c/66322] Linus Torvalds: -Wswitch-bool produces dubious warnings, fails to notice really bad things

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66322

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dcb314 at hotmail dot com

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 103983 has been marked as a duplicate of this bug. ***