From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754164Ab2DWO4d (ORCPT ); Mon, 23 Apr 2012 10:56:33 -0400 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:52414 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754013Ab2DWO4b (ORCPT ); Mon, 23 Apr 2012 10:56:31 -0400 Message-ID: <1335192983.4191.25.camel@dabdike.lan> Subject: Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro From: James Bottomley To: Paul Gortmaker Cc: Stephen Cameron , "Stephen M. Cameron" , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, matthew.gates@hp.com, thenzl@redhat.com, akpm@linux-foundation.org, mikem@beardog.cce.hp.com Date: Mon, 23 Apr 2012 15:56:23 +0100 In-Reply-To: <4F955F78.8070604@windriver.com> References: <20120420150349.10596.73732.stgit@beardog.cce.hp.com> <20120420150732.10596.9085.stgit@beardog.cce.hp.com> <4F955F78.8070604@windriver.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.1 Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2012-04-23 at 09:56 -0400, Paul Gortmaker wrote: > On 12-04-22 10:20 PM, Stephen Cameron wrote: > > On Sun, Apr 22, 2012 at 1:12 PM, Paul Gortmaker > > wrote: > >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron > >> wrote: > >>> From: Stephen M. Cameron > >>> > >>> Signed-off-by: Stephen M. Cameron > >> > >> You've not written a commit log, so I'm left guessing what the > >> intended rationale is here. COMPAT, X86 and PCI_MSI are > >> I believe all bool, so why make this change? To me it gives > >> a misleading message that some level of modular awareness > >> is needed here, when there really isn't any such need. > > > > Well, I saw this commit go by: 69349c2dc01c489eccaa4c472542c08e370c6d7e > > > > Using IS_ENABLED() within C (vs. within CPP #if statements) in its > > current form requires us to actually define every possible bool/tristate > > Kconfig option twice (__enabled_* and __enabled_*_MODULE variants). > > [...] > > > > and so I kind of thought IS_ENABLED is the new way to do that sort of thing. I don't think you need to change the driver unless there's a good reason. In kernel terms, it's usually better to wait a bit to see if the wheels fall off any particular bandwagon before leaping on it ... > In my opinion, IS_ENABLED can/should be used when you have: > > #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE) > > i.e. "is this driver built in, or can it be loaded as a module?" > The CONFIG_FOO_MODULE doesn't appear in your .config -- it is auto > generated by Kbuild infrastructure. > > It will do the Right Thing even for cases where CONFIG_FOO_MODULE > is impossible, but it does as I've said, give the wrong impression > that there is some possibility of modular presence. At least to > those who are familiar with the implementation and why it exists. > [I'll grant you that the name doesn't convey the use case too well.] > > > > > Perhaps I'm wrong about that. Obviously the patch is not _needed_ for > > things to work. > > I don't think we want to go and mass replace all existing cases of > > #ifdef CONFIG_SOME_BOOL > > with: > > #if IS_ENABLED(CONFIG_SOME_BOOL) > > There is no value add. However, replacing instances of: > > #if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE) > > with: > > #if IS_ENABLED(CONFIG_FOO) > > is in my opinion, a reasonable thing to do. It is shorter, and > it does hide the internally generated _MODULE suffixed "shadow" > variables from appearing directly in the main source files. And > it tells you that the author was thinking about both the built > in and the modular cases when they wrote it. To be honest, I think we want to use #if IS_ENABLED() for all cases going forwards, including the boolean case. The reason is that it's an easier design pattern. If we have the #ifdef CONFIG_X vs #if IS_ENABLED(CONFIG_X) depending on whether CONFIG_X can be modular or not it's just creating pointless rules that someone will annoy us all by enforcing in a checkpatch or some other script. Whereas #if IS_ENABLED(CONFIG_X) is obvious and simple. James