Message-ID: <4F96C38E.9000102@windriver.com>
Date:	Tue, 24 Apr 2012 11:15:26 -0400
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	James Bottomley <James.Bottomley@...senpartnership.com>
CC:	Stephen Cameron <stephenmcameron@...il.com>,
	"Stephen M. Cameron" <scameron@...rdog.cce.hp.com>,
	<linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<matthew.gates@...com>, <thenzl@...hat.com>,
	<akpm@...ux-foundation.org>, <mikem@...rdog.cce.hp.com>
Subject: Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro

On 12-04-24 04:25 AM, James Bottomley wrote:
> 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@...senpartnership.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@...driver.com> wrote:
>>>>>> On Fri, Apr 20, 2012 at 11:07 AM, Stephen M. Cameron
>>>>>> <scameron@...rdog.cce.hp.com> wrote:
>>>>>>> From: Stephen M. Cameron <scameron@...rdog.cce.hp.com>
>>>>>>>
>>>>>>> Signed-off-by: Stephen M. Cameron <scameron@...rdog.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

Again I'll have to disagree with you categorizing them as "arbitrary".

> 
> 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.

So, something in you doesn't go "eeech" when you see:

#if IS_ENABLED(CONFIG_ARM)

...that treats ARM as if it is some kind of driver, or feature
that can simply be enabled at will, alongside of any other
features?  Or worse, when people take it to the next level
and do:

-#ifdef CONFIG_SPARC
-	boo();
-	bar();
-	baz();
-#endif
+	if (IS_ENABLED(CONFIG_SPARC)) {
+		boo();
+		bar();
+		baz();
+	}

These are kinds of instances that I hope we don't entertain
as being improvements, or use cases we'd adopt as defaults.

> 
>> 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.

Hopefully the above makes my concerns more clear -- IS_ENABLED should
not be used blindly without thought.  Some things like core arch configs
will never _ever_ be modular.  Seeing IS_ENABLED used on those kinds of
options just seems ugly to me.  That is what was in this original patch.

But using it on some driver that isn't currently modular, but might be
made so eventually is reasonable.  Maybe you will still call that
distinction arbitrary, and the arch use case not ugly enough to matter.

Nor IMHO should IS_ENABLED be used as a tool to engage a holy war on
eradicating all #ifdef/#endif block.  Just like "goto", sensible uses
of such blocks can make things more readable, not less.

Your point earlier about recommending people show restraint before
rushing out to use the shiny new tool just because they can, was
a good one -- I agree wholeheartedly with that.

Thanks,
Paul.

> 
> James
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists