Messages in this thread | | | Date | Wed, 27 May 2015 15:32:15 -0700 | Subject | block: new gcc-5.1 warnings.. | From | 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
| |