lkml.org 
[lkml]   [2015]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: block: new gcc-5.1 warnings..
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



\
 
 \ /
  Last update: 2015-05-28 02:01    [W:0.071 / U:2.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site