Messages in this thread | | | Date | Wed, 27 May 2015 17:16:19 -0600 | From | Jens Axboe <> | Subject | Re: 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
| |