block: new gcc-5.1 warnings..

block: new gcc-5.1 warnings..

[Linus Torvalds]

So gcc-5.1 seems to have a few new warnings, most of which seem of
dubious value, but whatever.

One of them

drivers/block/hd.c: In function ‘hd_request’:
drivers/block/hd.c:630:11: warning: switch condition has boolean value
[-Wswitch-bool]
   switch (rq_data_dir(req)) {
           ^

just made me go "what?" since doing a switch on a boolean is perfectly
fine, and there can be various valid reasons to do so (using "break"
and fall-through etc can make the structure of the true/false cases
nicer).

So the compiler warning is just silly and stupid.

The warning would make more sense (and still trigger for this kernel
case) if the case statements then didn't use boolean values.

So despite the warning in general just being insane, in this case it
happens to show an oddity of the kernel source code: rq_data_dir()
returns a boolean, and that actually makes little sense, since it's
normally compared to READ/WRITE. Which *happen* to be 0/1, and integer
promotion does the right thing, but if you actually look at what
READ/WRITE are, it really is 0/1, not false/true.

This odd boolean came in through commit 5953316dbf90 ("block: make
rq->cmd_flags be 64-bit") and I think that change really was
questionable. What happened was that "cmd_flags" got turned into
"u64", and that commit wants to avoid making rq_data_dir() return a
u64, because that screws up printk() and friends.

But I think it might be better off as (I didn't test this):

 #define rq_data_dir(rq)                ((int)((rq)->cmd_flags & 1))

instead, to match the type of READ/WRITE. That would also get rid of
the (bogus) warning introduced by gcc-5.1.1.

And maybe somebody could then convince the gcc people that

   switch (boolean) {
   case true:
       ...
   case false:
   }

is actually perfectly fine.  It could still complain about the truly
odd cases (which the kernel use really arguably is).

Hmm? Jens?

               Linus

Re: block: new gcc-5.1 warnings..

[Kirill A. Shutemov]

On Wed, May 27, 2015 at 03:32:15PM -0700, Linus Torvalds wrote:
> So gcc-5.1 seems to have a few new warnings, most of which seem of
> dubious value, but whatever.
> 
> One of them
> 
> drivers/block/hd.c: In function ‘hd_request’:
> drivers/block/hd.c:630:11: warning: switch condition has boolean value
> [-Wswitch-bool]
>    switch (rq_data_dir(req)) {
>            ^
> 
> just made me go "what?" since doing a switch on a boolean is perfectly
> fine, and there can be various valid reasons to do so (using "break"
> and fall-through etc can make the structure of the true/false cases
> nicer).

In which situation fall-through switch() would make better structure then
plain if()? It's easier to miss-read fall-through.

-- 
 Kirill A. Shutemov

Re: block: new gcc-5.1 warnings..

[Jens Axboe]

On 05/27/2015 04:32 PM, Linus Torvalds wrote:
> So gcc-5.1 seems to have a few new warnings, most of which seem of
> dubious value, but whatever.
>
> One of them
>
> drivers/block/hd.c: In function ‘hd_request’:
> drivers/block/hd.c:630:11: warning: switch condition has boolean value
> [-Wswitch-bool]
>     switch (rq_data_dir(req)) {
>             ^
>
> just made me go "what?" since doing a switch on a boolean is perfectly
> fine, and there can be various valid reasons to do so (using "break"
> and fall-through etc can make the structure of the true/false cases
> nicer).
>
> So the compiler warning is just silly and stupid.
>
> The warning would make more sense (and still trigger for this kernel
> case) if the case statements then didn't use boolean values.
>
> So despite the warning in general just being insane, in this case it
> happens to show an oddity of the kernel source code: rq_data_dir()
> returns a boolean, and that actually makes little sense, since it's
> normally compared to READ/WRITE. Which *happen* to be 0/1, and integer
> promotion does the right thing, but if you actually look at what
> READ/WRITE are, it really is 0/1, not false/true.
>
> This odd boolean came in through commit 5953316dbf90 ("block: make
> rq->cmd_flags be 64-bit") and I think that change really was
> questionable. What happened was that "cmd_flags" got turned into
> "u64", and that commit wants to avoid making rq_data_dir() return a
> u64, because that screws up printk() and friends.
>
> But I think it might be better off as (I didn't test this):
>
>   #define rq_data_dir(rq)                ((int)((rq)->cmd_flags & 1))

That'd work just fine.

> instead, to match the type of READ/WRITE. That would also get rid of
> the (bogus) warning introduced by gcc-5.1.1.
>
> And maybe somebody could then convince the gcc people that
>
>     switch (boolean) {
>     case true:
>         ...
>     case false:
>     }
>
> is actually perfectly fine.  It could still complain about the truly
> odd cases (which the kernel use really arguably is).
>
> Hmm? Jens?

The case you quoted is arguably crap, since the default case can never 
be hit. I'm assuming this code dates back a long time, from when we 
checked the actual command type instead of a bit being set or not. Now, 
I don't have gcc-5.1 here, and the warning does make it seem like this 
is not what it's complaining about. So I'd be fine with just making 
rq_data_dir() return the right type and get rid of the != 0.

-- 
Jens Axboe

Re: block: new gcc-5.1 warnings..

[Linus Torvalds]

On Wed, May 27, 2015 at 3:56 PM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
>
> In which situation fall-through switch() would make better structure then
> plain if()? It's easier to miss-read fall-through.

"switch()" is rather more expressive than "if ()", mostly thanks to "break".

With switch(), you can write things like

    switch (a) {
    case 1:
        A;
        if (b)
             break;
        B;
        /* fallthrough */
    case 2:
        C;
    }

where you share that "C" case for some conditions but not others.

You can do the same using goto, of course, but you can *not* do it
with pure nested "if ()" statements.

So even with just two cases, "switch ()" is syntactically more
powerful than "if ()" because it allows more structured exits.

So anybody who says that "booleans don't make sense for switch()" is just crazy.

Now, the reason the warning actually happened to make sense for the
kernel wasn't because it was a boolean, but because the type of the
switch and the case statements didn't match.

So I do actually agree that

    switch (boolean) {
    case non-boolean:

can very much be worth a warning. But then it's about type-safety
issues, rather than about "you shouldn't use switch() with a boolean".

See the difference?

                 Linus

Re: block: new gcc-5.1 warnings..

[Linus Torvalds]

On Wed, May 27, 2015 at 4:18 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> So I do actually agree that
>
>     switch (boolean) {
>     case non-boolean:
>
> can very much be worth a warning. But then it's about type-safety
> issues, rather than about "you shouldn't use switch() with a boolean".

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.

              Linus