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

[Paul Gortmaker <paul.gortmaker@windriver.com>]

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.

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.

Paul.
--

> 
> -- steve