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 > > <paul.gortmaker@xxxxxxxxxxxxx> wrote: > >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron > >> <scameron@xxxxxxxxxxxxxxxxxx> wrote: > >>> From: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx> > >>> > >>> Signed-off-by: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx> > >> > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
