lkml.org 
[lkml]   [2012]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 14/17] hpsa: use new IS_ENABLED macro
From
Date
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




\
 
 \ /
  Last update: 2012-04-23 16:59    [W:0.157 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site