Messages in this thread | | | Subject | Re: [PATCH 14/17] hpsa: use new IS_ENABLED macro | From | James Bottomley <> | Date | Tue, 24 Apr 2012 09:25:10 +0100 |
| |
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
| |