From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753222AbbE0XQX (ORCPT ); Wed, 27 May 2015 19:16:23 -0400 Received: from mail-pd0-f176.google.com ([209.85.192.176]:35030 "EHLO mail-pd0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752319AbbE0XQV (ORCPT ); Wed, 27 May 2015 19:16:21 -0400 Message-ID: <55665043.1040904@kernel.dk> Date: Wed, 27 May 2015 17:16:19 -0600 From: Jens Axboe User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Linus Torvalds CC: Linux Kernel Mailing List Subject: Re: block: new gcc-5.1 warnings.. References: In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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