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

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

On Mon, 2012-04-23 at 21:43 -0400, Paul Gortmaker wrote:
> On Mon, Apr 23, 2012 at 10:56 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 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.
> 
> I guess we will have to agree to disagree then.
> 
> >
> > 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.
> 
> What exactly is an "easier design pattern", and what are the gains?

It means you only have one rule:

everything is #if IS_ENABLED

vs two arbitrary ones

If it's a boolean symbol, you use #ifdef else if it's tristate, use #if
IS_ENABLED

It's obvious to me the former is simpler.

> To me, it has nothing to do with rules, and what the checkpatch folks do
> or do not do.  The line "#ifdef CONFIG_FOO" conveys one specific piece
> of information.  The line "#if IS_ENABLED(CONFIG_FOO) conveys
> a different piece of information, which is a superset of the former.

That's the point ... why bother with the former?  If the latter is a
superset and you always do the latter, the rule is simpler.

> If you blindly convert all of them, you throw away information that would
> otherwise be available to the code reader.  I would not support that.

Well, a) I'm not advocating converting everything.  I'm advocating using
the superset for everything going forwards.

I don't understand what information would be lost:  All use if #ifdef vs
#if IS_ENABLED tells you is whether the author thought the symbol was
boolean or tristate.  I don't see how that's useful at all and it will
cause problems if a symbol changes (some non-modular code may become
modular for instance) ... then we have to go and chase it through the
code base.

James