Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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@windriver.com> wrote:
> >> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
> >> <scameron@beardog.cce.hp.com> wrote:
> >>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>>
> >>> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>
> >> 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