kernel coding style for if ... else which cross #ifdef

kernel coding style for if ... else which cross #ifdef

[Steve French]

A question splitting "else" and "if" on distinct lines vs. using an
extra line and extra #else came up as I was reviewing a proposed cifs
patch.   Which is the preferred style?

#ifdef CONFIG_SOMETHING
   if (foo)
      something ...
   else
#endif
   if ((mode & S_IWUGO) == 0)

or alternatively

#ifdef CONFIG_SOMETHING
   if (foo)
      something ...
   else if ((mode & S_IWUGO) == 0)
#else
   if ((mode & S_IWUGO) == 0)
#endif


-- 
Thanks,

Steve

Re: kernel coding style for if ... else which cross #ifdef

[Willy Tarreau]

On Fri, May 23, 2008 at 02:11:43PM -0500, Steve French wrote:
> A question splitting "else" and "if" on distinct lines vs. using an
> extra line and extra #else came up as I was reviewing a proposed cifs
> patch.   Which is the preferred style?
>
> #ifdef CONFIG_SOMETHING
>    if (foo)
>       something ...
>    else
> #endif
>    if ((mode & S_IWUGO) == 0)
> 
> or alternatively
> 
> #ifdef CONFIG_SOMETHING
>    if (foo)
>       something ...
>    else if ((mode & S_IWUGO) == 0)
> #else
>    if ((mode & S_IWUGO) == 0)
> #endif

The second one is dangerous because if code evolves, chances are that
only one of the two identical lines will be updated.

At least the first one is clearly readable. But if you have tons of
places with the same construct, it's better to create a macro which
will inhibit the if branch, which gcc will happily optimize away.
For instance :

#ifdef CONFIG_FOO
#define FOO_ENABLED 1
#else
#define FOO_ENABLED 0
#endif

if (FOO_ENABLED && foo)
    something
else if ((mode & S_IWUGO) == 0)
   ...

One variant includes :

#ifdef CONFIG_FOO
#define FOO_COND(x) (x)
#else
#define FOO_COND(x) 0
#endif

if (FOO_COND(foo))
    something
else if ((mode & S_IWUGO) == 0)
   ...

Regards,
Willy

Re: kernel coding style for if ... else which cross #ifdef

[Adrian Bunk]

On Fri, May 23, 2008 at 10:42:28PM +0200, Willy Tarreau wrote:
> On Fri, May 23, 2008 at 02:11:43PM -0500, Steve French wrote:
> > A question splitting "else" and "if" on distinct lines vs. using an
> > extra line and extra #else came up as I was reviewing a proposed cifs
> > patch.   Which is the preferred style?
> >
> > #ifdef CONFIG_SOMETHING
> >    if (foo)
> >       something ...
> >    else
> > #endif
> >    if ((mode & S_IWUGO) == 0)
> > 
> > or alternatively
> > 
> > #ifdef CONFIG_SOMETHING
> >    if (foo)
> >       something ...
> >    else if ((mode & S_IWUGO) == 0)
> > #else
> >    if ((mode & S_IWUGO) == 0)
> > #endif
> 
> The second one is dangerous because if code evolves, chances are that
> only one of the two identical lines will be updated.
> 
> At least the first one is clearly readable.

I would consider the first one much harder to read since you can _very_ 
easily miss that the "if" is in an "else" clause and completely misread 
the code.

> But if you have tons of
> places with the same construct, it's better to create a macro which
> will inhibit the if branch, which gcc will happily optimize away.
> For instance :
> 
> #ifdef CONFIG_FOO
> #define FOO_ENABLED 1
> #else
> #define FOO_ENABLED 0
> #endif
> 
> if (FOO_ENABLED && foo)
>     something
> else if ((mode & S_IWUGO) == 0)
>    ...
>...

I'd also say that's the best solution.

> Regards,
> Willy

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: kernel coding style for if ... else which cross #ifdef

[Willy Tarreau]

On Fri, May 23, 2008 at 11:49:12PM +0300, Adrian Bunk wrote:
> On Fri, May 23, 2008 at 10:42:28PM +0200, Willy Tarreau wrote:
> > On Fri, May 23, 2008 at 02:11:43PM -0500, Steve French wrote:
> > > A question splitting "else" and "if" on distinct lines vs. using an
> > > extra line and extra #else came up as I was reviewing a proposed cifs
> > > patch.   Which is the preferred style?
> > >
> > > #ifdef CONFIG_SOMETHING
> > >    if (foo)
> > >       something ...
> > >    else
> > > #endif
> > >    if ((mode & S_IWUGO) == 0)
> > > 
> > > or alternatively
> > > 
> > > #ifdef CONFIG_SOMETHING
> > >    if (foo)
> > >       something ...
> > >    else if ((mode & S_IWUGO) == 0)
> > > #else
> > >    if ((mode & S_IWUGO) == 0)
> > > #endif
> > 
> > The second one is dangerous because if code evolves, chances are that
> > only one of the two identical lines will be updated.
> > 
> > At least the first one is clearly readable.
> 
> I would consider the first one much harder to read since you can _very_ 
> easily miss that the "if" is in an "else" clause and completely misread 
> the code.

speaking about readability, yes I agree (the second "if" ought to be
indented for the eye to catch it). I'm worrying about the risk of
code being asymetrically added between the two ifs in the second form.

> > But if you have tons of
> > places with the same construct, it's better to create a macro which
> > will inhibit the if branch, which gcc will happily optimize away.
> > For instance :
> > 
> > #ifdef CONFIG_FOO
> > #define FOO_ENABLED 1
> > #else
> > #define FOO_ENABLED 0
> > #endif
> > 
> > if (FOO_ENABLED && foo)
> >     something
> > else if ((mode & S_IWUGO) == 0)
> >    ...
> >...
> 
> I'd also say that's the best solution.

When we both agree on something, it must be a really good solution :-)

> cu
> Adrian

Cheers,
Willy

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Steve French wrote:
> A question splitting "else" and "if" on distinct lines vs. using an
> extra line and extra #else came up as I was reviewing a proposed cifs
> patch.   Which is the preferred style?
> 
> #ifdef CONFIG_SOMETHING
>    if (foo)
>       something ...
>    else
> #endif
>    if ((mode & S_IWUGO) == 0)
> 
> or alternatively
> 
> #ifdef CONFIG_SOMETHING
>    if (foo)
>       something ...
>    else if ((mode & S_IWUGO) == 0)
> #else
>    if ((mode & S_IWUGO) == 0)
> #endif
> 

The former.  Why?  Because the latter case has unbalanced indentation: 
to an editor, and to the human eye, it looks like the if in the #else 
clause is a child to the "else if".

*However*, the best would really be if we changed Kconfig to emit 
configuration constants what were 0/1 instead of undefined/defined. 
That way we could do:

	if (CONFIG_SOMETHING && foo) {
		/* ... something ... */
	} else if ((mode & S_IWUGO) == 0) {
		/* ... */

... in many cases.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[Sam Ravnborg]

> *However*, the best would really be if we changed Kconfig to emit 
> configuration constants what were 0/1 instead of undefined/defined. 
> That way we could do:
> 
> 	if (CONFIG_SOMETHING && foo) {
> 		/* ... something ... */
> 	} else if ((mode & S_IWUGO) == 0) {
> 		/* ... */

We could do that - but then it would need another
name not to clash with all the places where we rely
on CONFIG_FOO='n' => CONFIG_FOO is not defined.

We could teach kconfig to emit something like:
#define KFOO 0   (for the 'n' value)
And 1 or 2 for the y and m values.

	Sam

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Sam Ravnborg wrote:
>> *However*, the best would really be if we changed Kconfig to emit 
>> configuration constants what were 0/1 instead of undefined/defined. 
>> That way we could do:
>>
>> 	if (CONFIG_SOMETHING && foo) {
>> 		/* ... something ... */
>> 	} else if ((mode & S_IWUGO) == 0) {
>> 		/* ... */
> 
> We could do that - but then it would need another
> name not to clash with all the places where we rely
> on CONFIG_FOO='n' => CONFIG_FOO is not defined.
> 
> We could teach kconfig to emit something like:
> #define KFOO 0   (for the 'n' value)
> And 1 or 2 for the y and m values.
> 

I don't think we want to use "1 or 2"... I suspect we want to use the 
same booleans we currently have.

I would suggest CFG_* instead of CONFIG_* for the new set.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[Sam Ravnborg]

On Fri, May 23, 2008 at 10:42:58PM -0700, H. Peter Anvin wrote:
> Sam Ravnborg wrote:
> >>*However*, the best would really be if we changed Kconfig to emit 
> >>configuration constants what were 0/1 instead of undefined/defined. 
> >>That way we could do:
> >>
> >>	if (CONFIG_SOMETHING && foo) {
> >>		/* ... something ... */
> >>	} else if ((mode & S_IWUGO) == 0) {
> >>		/* ... */
> >
> >We could do that - but then it would need another
> >name not to clash with all the places where we rely
> >on CONFIG_FOO='n' => CONFIG_FOO is not defined.
> >
> >We could teach kconfig to emit something like:
> >#define KFOO 0   (for the 'n' value)
> >And 1 or 2 for the y and m values.
> >
> 
> I don't think we want to use "1 or 2"... I suspect we want to use the 
> same booleans we currently have.
I'm a bit dense (or I need more coffe - it's morning here).
What "same booleans"?

> I would suggest CFG_* instead of CONFIG_* for the new set.
Agreed.

	Sam

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Sam Ravnborg wrote:
> On Fri, May 23, 2008 at 10:42:58PM -0700, H. Peter Anvin wrote:
>   
>> Sam Ravnborg wrote:
>>     
>>>> *However*, the best would really be if we changed Kconfig to emit 
>>>> configuration constants what were 0/1 instead of undefined/defined. 
>>>> That way we could do:
>>>>
>>>> 	if (CONFIG_SOMETHING && foo) {
>>>> 		/* ... something ... */
>>>> 	} else if ((mode & S_IWUGO) == 0) {
>>>> 		/* ... */
>>>>         
>>> We could do that - but then it would need another
>>> name not to clash with all the places where we rely
>>> on CONFIG_FOO='n' => CONFIG_FOO is not defined.
>>>
>>> We could teach kconfig to emit something like:
>>> #define KFOO 0   (for the 'n' value)
>>> And 1 or 2 for the y and m values.
>>>
>>>       
>> I don't think we want to use "1 or 2"... I suspect we want to use the 
>> same booleans we currently have.
>>     
> I'm a bit dense (or I need more coffe - it's morning here).
> What "same booleans"?
>   

They should be plain 0/1 booleans.  For a bool/tristate option FOO, it 
would define:

Enabled y:

    #define CONFIG_FOO
    #define CFG_FOO   1
    #undef CONFIG_FOO_MODULE
    #define CFG_FOO_MODULE 0


Enabled m:

    #define CONFIG_FOO
    #define CFG_FOO   1
    #define CONFIG_FOO_MODULE
    #define CFG_FOO_MODULE 1


Disabled n:

    #undef CONFIG_FOO
    #define CFG_FOO   0
    #undef CONFIG_FOO_MODULE
    #define CFG_FOO_MODULE 0

Not sure what CFG_* should be for string/numeric options.  Probably "1" 
if the value is defined, "0" if not, with CONFIG_* being the actual 
value (so a CONFIG_ value of 0 is distinguishable from not defined).

    J

Re: kernel coding style for if ... else which cross #ifdef

[Adrian Bunk]

On Sat, May 24, 2008 at 11:06:21AM +0100, Jeremy Fitzhardinge wrote:
> Sam Ravnborg wrote:
>> On Fri, May 23, 2008 at 10:42:58PM -0700, H. Peter Anvin wrote:
>>   
>>> Sam Ravnborg wrote:
>>>     
>>>>> *However*, the best would really be if we changed Kconfig to emit 
>>>>> configuration constants what were 0/1 instead of 
>>>>> undefined/defined. That way we could do:
>>>>>
>>>>> 	if (CONFIG_SOMETHING && foo) {
>>>>> 		/* ... something ... */
>>>>> 	} else if ((mode & S_IWUGO) == 0) {
>>>>> 		/* ... */
>>>>>         
>>>> We could do that - but then it would need another
>>>> name not to clash with all the places where we rely
>>>> on CONFIG_FOO='n' => CONFIG_FOO is not defined.
>>>>
>>>> We could teach kconfig to emit something like:
>>>> #define KFOO 0   (for the 'n' value)
>>>> And 1 or 2 for the y and m values.
>>>>
>>>>       
>>> I don't think we want to use "1 or 2"... I suspect we want to use the 
>>> same booleans we currently have.
>>>     
>> I'm a bit dense (or I need more coffe - it's morning here).
>> What "same booleans"?
>>   
>
> They should be plain 0/1 booleans.  For a bool/tristate option FOO, it  
> would define:
>
> Enabled y:
>
>    #define CONFIG_FOO
>    #define CFG_FOO   1
>    #undef CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 0
>
>
> Enabled m:
>
>    #define CONFIG_FOO
>    #define CFG_FOO   1
>    #define CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 1
>...

A quite common pattern in the kernel is:
#if defined(CONFIG_FOO) || (defined(CONFIG_FOO_MODULE) && defined(MODULE))

Your suggestion would require them to be changed to:
#if (defined(CONFIG_FOO) && !defined(CONFIG_FOO_MODULE)) || (defined(CONFIG_FOO_MODULE) && defined(MODULE))

(We could push these cases to kconfig, but there might also be other 
 cases where changing the existing semantics of CONFIG_FOO could cause
 breakages.)

We see daily in kconfig that mixing tristates with bools is tricky 
(especially since bools are used with different intended semantics),
and I don't think doing the same in the source files would be an 
improvement.

We might be able to do (without any CFG_FOO_MODULE at all):

Enabled m:

    #undef CONFIG_FOO
    #define CFG_FOO   0
    #define CONFIG_FOO_MODULE

And let everyone who want's "either y or m" semantics to convert the 
tristate to a bool with this semantics in kconfig himself:

config FOO
	tristate

config BAR
	def_bool FOO

Especially since this is actually a relatively unusual (and not nice) 
case since usually adding a module does not (and should) not change the 
kernel image.

>    J

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: kernel coding style for if ... else which cross #ifdef

[Sam Ravnborg]

On Sat, May 24, 2008 at 11:06:21AM +0100, Jeremy Fitzhardinge wrote:
> Sam Ravnborg wrote:
> >On Fri, May 23, 2008 at 10:42:58PM -0700, H. Peter Anvin wrote:
> >  
> >>Sam Ravnborg wrote:
> >>    
> >>>>*However*, the best would really be if we changed Kconfig to emit 
> >>>>configuration constants what were 0/1 instead of undefined/defined. 
> >>>>That way we could do:
> >>>>
> >>>>	if (CONFIG_SOMETHING && foo) {
> >>>>		/* ... something ... */
> >>>>	} else if ((mode & S_IWUGO) == 0) {
> >>>>		/* ... */
> >>>>        
> >>>We could do that - but then it would need another
> >>>name not to clash with all the places where we rely
> >>>on CONFIG_FOO='n' => CONFIG_FOO is not defined.
> >>>
> >>>We could teach kconfig to emit something like:
> >>>#define KFOO 0   (for the 'n' value)
> >>>And 1 or 2 for the y and m values.
> >>>
> >>>      
> >>I don't think we want to use "1 or 2"... I suspect we want to use the 
> >>same booleans we currently have.
> >>    
> >I'm a bit dense (or I need more coffe - it's morning here).
> >What "same booleans"?
> >  
> 
> They should be plain 0/1 booleans.  For a bool/tristate option FOO, it 
> would define:
> 
> Enabled y:
> 
>    #define CONFIG_FOO
>    #define CFG_FOO   1
>    #undef CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 0
Agreed

> 
> Enabled m:
> 
>    #define CONFIG_FOO
>    #define CFG_FOO   1
>    #define CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 1

I assume you wanted to say:
>    #undef  CONFIG_FOO
>    #define CFG_FOO   1
>    #define CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 1
Because then the CONFIG_* is not changed
and we do not want to change that.

I'm not fully convinced about:
>    #define CFG_FOO   1
But on the other hand it is only in odd
cases we distingush between built-in and module.
So it makes most sense.

> Disabled n:
> 
>    #undef CONFIG_FOO
>    #define CFG_FOO   0
>    #undef CONFIG_FOO_MODULE
>    #define CFG_FOO_MODULE 0
Agree.

> 
> Not sure what CFG_* should be for string/numeric options.  Probably "1" 
> if the value is defined, "0" if not, with CONFIG_* being the actual 
> value (so a CONFIG_ value of 0 is distinguishable from not defined).
For non-boolean/tristate values we simply skip CFG_ values - thats
the most simple approach.

	Sam

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Sam Ravnborg wrote:
> I assume you wanted to say:
>   
>>    #undef  CONFIG_FOO
>>    #define CFG_FOO   1
>>    #define CONFIG_FOO_MODULE
>>    #define CFG_FOO_MODULE 1
>>     
> Because then the CONFIG_* is not changed
> and we do not want to change that.
>   

Yeah, I didn't intend to change the meaning of CONFIG_FOO.

> I'm not fully convinced about:
>   
>>    #define CFG_FOO   1
>>     
> But on the other hand it is only in odd
> cases we distingush between built-in and module.
> So it makes most sense.
>   

I think CONFIG_ and CFG_ should be exact parallels, so if CONFIG_FOO is 
undefined, CFG_FOO should be 0.

>> Not sure what CFG_* should be for string/numeric options.  Probably "1" 
>> if the value is defined, "0" if not, with CONFIG_* being the actual 
>> value (so a CONFIG_ value of 0 is distinguishable from not defined).
>>     
> For non-boolean/tristate values we simply skip CFG_ values - thats
> the most simple approach.

I suppose, but it might be useful to know whether a constant is present:

	if (CFG_THINGY_LIMIT && x > CONFIG_THINGY_LIMIT) {...}

(which fails if CONFIG_THINGY_LIMIT is undefined, so I guess it still 
doesn't work very well).

    J

Re: kernel coding style for if ... else which cross #ifdef

[Willy Tarreau]

On Sat, May 24, 2008 at 03:35:58PM +0100, Jeremy Fitzhardinge wrote:
> Sam Ravnborg wrote:
> >I assume you wanted to say:
> >  
> >>   #undef  CONFIG_FOO
> >>   #define CFG_FOO   1
> >>   #define CONFIG_FOO_MODULE
> >>   #define CFG_FOO_MODULE 1
> >>    
> >Because then the CONFIG_* is not changed
> >and we do not want to change that.
> >  
> 
> Yeah, I didn't intend to change the meaning of CONFIG_FOO.
> 
> >I'm not fully convinced about:
> >  
> >>   #define CFG_FOO   1
> >>    
> >But on the other hand it is only in odd
> >cases we distingush between built-in and module.
> >So it makes most sense.
> >  
> 
> I think CONFIG_ and CFG_ should be exact parallels, so if CONFIG_FOO is 
> undefined, CFG_FOO should be 0.
> 
> >>Not sure what CFG_* should be for string/numeric options.  Probably "1" 
> >>if the value is defined, "0" if not, with CONFIG_* being the actual 
> >>value (so a CONFIG_ value of 0 is distinguishable from not defined).
> >>    
> >For non-boolean/tristate values we simply skip CFG_ values - thats
> >the most simple approach.
> 
> I suppose, but it might be useful to know whether a constant is present:
> 
> 	if (CFG_THINGY_LIMIT && x > CONFIG_THINGY_LIMIT) {...}
> 
> (which fails if CONFIG_THINGY_LIMIT is undefined, so I guess it still 
> doesn't work very well).

You still have the possibility to use the "-0" trick :

 	if (CFG_THINGY_LIMIT && x > (CONFIG_THINGY_LIMIT-0)) {...}

Willy

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Willy Tarreau wrote:
> You still have the possibility to use the "-0" trick :
>
>  	if (CFG_THINGY_LIMIT && x > (CONFIG_THINGY_LIMIT-0)) {...}
>   

Oh, that's cute in a vile way.  I hadn't seen it before.

    J

Re: kernel coding style for if ... else which cross #ifdef

[Willy Tarreau]

On Sat, May 24, 2008 at 03:41:21PM +0100, Jeremy Fitzhardinge wrote:
> Willy Tarreau wrote:
> >You still have the possibility to use the "-0" trick :
> >
> > 	if (CFG_THINGY_LIMIT && x > (CONFIG_THINGY_LIMIT-0)) {...}
> >  
> 
> Oh, that's cute in a vile way.  I hadn't seen it before.

I use it in some of my code (but moderately). It's particularly useful
for limit checking such as above. It's useful too when you want to use
the config value as an offset or a bit shift. Eg:

    p = malloc(buf_size << (CONFIG_SHIFT-0));

Willy

Re: kernel coding style for if ... else which cross #ifdef

[Sam Ravnborg]

On Sat, May 24, 2008 at 03:35:58PM +0100, Jeremy Fitzhardinge wrote:
> Sam Ravnborg wrote:
> >I assume you wanted to say:
> >  
> >>   #undef  CONFIG_FOO
> >>   #define CFG_FOO   1
> >>   #define CONFIG_FOO_MODULE
> >>   #define CFG_FOO_MODULE 1
> >>    
> >Because then the CONFIG_* is not changed
> >and we do not want to change that.
> >  
> 
> Yeah, I didn't intend to change the meaning of CONFIG_FOO.
> 
> >I'm not fully convinced about:
> >  
> >>   #define CFG_FOO   1
> >>    
> >But on the other hand it is only in odd
> >cases we distingush between built-in and module.
> >So it makes most sense.
> >  
> 
> I think CONFIG_ and CFG_ should be exact parallels, so if CONFIG_FOO is 
> undefined, CFG_FOO should be 0.

We should actually do as you intially suggested and alwyas
define CONFIG_FOO no matter if FOO is built-in or module.
Because we do only want to distingush between the two in rare cases.

But that is a separate patch and lets not do the same
mistage with CFG_*
I cooked up following patch - but I have not test-build a kernel yet.
We may use CFG_* here and there and clash is not good.

	Sam

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index ee5fe94..98a2c39 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -717,15 +717,21 @@ int conf_write_autoconf(void)
 		case S_BOOLEAN:
 		case S_TRISTATE:
 			switch (sym_get_tristate_value(sym)) {
-			case no:
-				break;
 			case mod:
 				fprintf(out, "CONFIG_%s=m\n", sym->name);
 				fprintf(out_h, "#define CONFIG_%s_MODULE 1\n", sym->name);
+				fprintf(out_h, "#define CFG_%s 1\n", sym->name);
+				fprintf(out_h, "#define CFG_%s_MODULE 1\n", sym->name);
 				break;
 			case yes:
 				fprintf(out, "CONFIG_%s=y\n", sym->name);
 				fprintf(out_h, "#define CONFIG_%s 1\n", sym->name);
+				fprintf(out_h, "#define CFG_%s 1\n", sym->name);
+				fprintf(out_h, "#define CFG_%s_MODULE 0\n", sym->name);
+				break;
+			case no:
+				fprintf(out_h, "#define CFG_%s 0\n", sym->name);
+				fprintf(out_h, "#define CFG_%s_MODULE 0\n", sym->name);
 				break;
 			}
 			break;

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Sam Ravnborg wrote:
> We should actually do as you intially suggested and alwyas
> define CONFIG_FOO no matter if FOO is built-in or module.
> Because we do only want to distingush between the two in rare cases.
>
> But that is a separate patch and lets not do the same
> mistage with CFG_*
>   

I think pretty strongly that CFG_ and CONFIG_ should be exactly 
parallel.  If you want to change the meaning of CONFIG_X in the presence 
of modules, then change CFG_X at the same time.  Making them have 
different meanings will just confuse anyone wanting to convert #ifdef 
CONFIG_ code into if(CFG_) code.

> I cooked up following patch - but I have not test-build a kernel yet.
> We may use CFG_* here and there and clash is not good.
>   

I have to say I'm not very keen on the CFG_* prefix.  It doesn't have 
any inherent meaning and just looks like a redundant abbreviation of 
CONFIG_; something which actually expresses the notion that it's always 
a compile-time constant would be better.  Not that I have any 
particularly good alternatives: CONST_? CCONST_? CONFIG_X_VAL? KCONFIG_? 
KONFIG_? KCONST_?

    J

Re: kernel coding style for if ... else which cross #ifdef

[Vegard Nossum]

On Sat, May 24, 2008 at 5:45 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> Sam Ravnborg wrote:
>>
>> We should actually do as you intially suggested and alwyas
>> define CONFIG_FOO no matter if FOO is built-in or module.
>> Because we do only want to distingush between the two in rare cases.
>>
>> But that is a separate patch and lets not do the same
>> mistage with CFG_*
>>
>
> I think pretty strongly that CFG_ and CONFIG_ should be exactly parallel.
>  If you want to change the meaning of CONFIG_X in the presence of modules,
> then change CFG_X at the same time.  Making them have different meanings
> will just confuse anyone wanting to convert #ifdef CONFIG_ code into
> if(CFG_) code.
>
>> I cooked up following patch - but I have not test-build a kernel yet.
>> We may use CFG_* here and there and clash is not good.
>>
>
> I have to say I'm not very keen on the CFG_* prefix.  It doesn't have any
> inherent meaning and just looks like a redundant abbreviation of CONFIG_;
> something which actually expresses the notion that it's always a
> compile-time constant would be better.  Not that I have any particularly
> good alternatives: CONST_? CCONST_? CONFIG_X_VAL? KCONFIG_? KONFIG_?
> KCONST_?

Don't know if this is really my place, but I could not agree more with
your characterisation of the CFG_* prefix and I will make the
following suggestion:

Why not use all-lowercase config_* names? It seems elegant, and fits
in with the notion that these are to be used not as macros, but as
ordinary constants.

(The only disadvantage I can see is that they will stand out less. But
I don't know how great the disadvantage is.)

You could even go further and make them real constants, something
along the lines of:

enum config_value { no, yes, mod };

static const enum config_value config_lockdep_support = yes;

(I believe the "static const" will prevent emission of this symbol in
the object file -- though I am not certain.)

You can now also test for specific values, e.g. if
(config_scsi_wait_scan == mod), in addition to simply testing the
truth value.

That concludes my suggestion.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Vegard Nossum wrote:
> Why not use all-lowercase config_* names? It seems elegant, and fits
> in with the notion that these are to be used not as macros, but as
> ordinary constants.
>   

We tend to use all caps for symbolic constants, even if they're enums.

> (The only disadvantage I can see is that they will stand out less. But
> I don't know how great the disadvantage is.)
>
> You could even go further and make them real constants, something
> along the lines of:
>
> enum config_value { no, yes, mod };
>
> static const enum config_value config_lockdep_support = yes;
>   

Well, you could use "enum { config_foo = 0/1 }"  to define a proper C 
constant.

But it means you could only use them in C, not in CPP or asm expressions.

    J

Re: kernel coding style for if ... else which cross #ifdef

[Tom Spink]

2008/5/24 Jeremy Fitzhardinge <jeremy@goop.org>:
> Vegard Nossum wrote:
>>
>> Why not use all-lowercase config_* names? It seems elegant, and fits
>> in with the notion that these are to be used not as macros, but as
>> ordinary constants.
>>
>
> We tend to use all caps for symbolic constants, even if they're enums.
>
>> (The only disadvantage I can see is that they will stand out less. But
>> I don't know how great the disadvantage is.)
>>
>> You could even go further and make them real constants, something
>> along the lines of:
>>
>> enum config_value { no, yes, mod };
>>
>> static const enum config_value config_lockdep_support = yes;
>>
>
> Well, you could use "enum { config_foo = 0/1 }"  to define a proper C
> constant.
>
> But it means you could only use them in C, not in CPP or asm expressions.
>
>   J

Hi,

A thought occurred to me that we may be able to used some preprocessor
magic and do this:

#define config_defined(x) CFGVAL_## x

Which means that, if we get Kconfig to produce:

#define CFGVAL_CONFIG_FOO 0
#define CFGVAL_CONFIG_VALUE_BAR 1
#define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1

We can use this:

if (config_defined(CONFIG_FOO) && some_expr) {
   panic("Oh no.");
}

Thoughts?

-- 
Tom Spink

Re: kernel coding style for if ... else which cross #ifdef

[Tom Spink]

2008/5/24 Tom Spink <tspink@gmail.com>:
> 2008/5/24 Jeremy Fitzhardinge <jeremy@goop.org>:
>> Vegard Nossum wrote:
>>>
>>> Why not use all-lowercase config_* names? It seems elegant, and fits
>>> in with the notion that these are to be used not as macros, but as
>>> ordinary constants.
>>>
>>
>> We tend to use all caps for symbolic constants, even if they're enums.
>>
>>> (The only disadvantage I can see is that they will stand out less. But
>>> I don't know how great the disadvantage is.)
>>>
>>> You could even go further and make them real constants, something
>>> along the lines of:
>>>
>>> enum config_value { no, yes, mod };
>>>
>>> static const enum config_value config_lockdep_support = yes;
>>>
>>
>> Well, you could use "enum { config_foo = 0/1 }"  to define a proper C
>> constant.
>>
>> But it means you could only use them in C, not in CPP or asm expressions.
>>
>>   J
>
> Hi,
>
> A thought occurred to me that we may be able to used some preprocessor
> magic and do this:
>
> #define config_defined(x) CFGVAL_## x
>
> Which means that, if we get Kconfig to produce:
>
> #define CFGVAL_CONFIG_FOO 0
> #define CFGVAL_CONFIG_VALUE_BAR 1
> #define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1
>
> We can use this:
>
> if (config_defined(CONFIG_FOO) && some_expr) {
>   panic("Oh no.");
> }
>
> Thoughts?
>
> --
> Tom Spink
>

There's a wee typo on the #define lines there:

> #define CFGVAL_CONFIG_FOO 0
> #define CFGVAL_CONFIG_VALUE_BAR 1
> #define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1

I, of course, meant:

#define CFGVAL_CONFIG_FOO 0
#define CFGVAL_CONFIG_BAR 1
#define CFGVAL_CONFIG_BAZ_MODULE 1

-- 
Tom Spink

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

Tom Spink wrote:
> A thought occurred to me that we may be able to used some preprocessor
> magic and do this:
>
> #define config_defined(x) CFGVAL_## x
>
> Which means that, if we get Kconfig to produce:
>
> #define CFGVAL_CONFIG_FOO 0
> #define CFGVAL_CONFIG_VALUE_BAR 1
> #define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1
>
> We can use this:
>
> if (config_defined(CONFIG_FOO) && some_expr) {
>    panic("Oh no.");
> }

Well, in that case you could use Willy's magic hack:

#define config_defined(x)      (x - 0)

Which isn't a bad alternative to defining a whole pile of new symbols...

    J

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> Tom Spink wrote:
>> A thought occurred to me that we may be able to used some preprocessor
>> magic and do this:
>>
>> #define config_defined(x) CFGVAL_## x
>>
>> Which means that, if we get Kconfig to produce:
>>
>> #define CFGVAL_CONFIG_FOO 0
>> #define CFGVAL_CONFIG_VALUE_BAR 1
>> #define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1
>>
>> We can use this:
>>
>> if (config_defined(CONFIG_FOO) && some_expr) {
>>    panic("Oh no.");
>> }
> 
> Well, in that case you could use Willy's magic hack:
> 
> #define config_defined(x)      (x - 0)
> 
> Which isn't a bad alternative to defining a whole pile of new symbols...
> 

That can *strongly* be argued with.

In particular, the use of #ifdef is crap to begin with.  Using #if even 
for the preprocessor makes it possible to trap misspellings.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

H. Peter Anvin wrote:
> Jeremy Fitzhardinge wrote:
>> Tom Spink wrote:
>>> A thought occurred to me that we may be able to used some preprocessor
>>> magic and do this:
>>>
>>> #define config_defined(x) CFGVAL_## x
>>>
>>> Which means that, if we get Kconfig to produce:
>>>
>>> #define CFGVAL_CONFIG_FOO 0
>>> #define CFGVAL_CONFIG_VALUE_BAR 1
>>> #define CFGVAL_CONFIG_VALUE_BAZ_MODULE 1
>>>
>>> We can use this:
>>>
>>> if (config_defined(CONFIG_FOO) && some_expr) {
>>>    panic("Oh no.");
>>> }
>>
>> Well, in that case you could use Willy's magic hack:
>>
>> #define config_defined(x)      (x - 0)
>>
>> Which isn't a bad alternative to defining a whole pile of new symbols...
>>
>
> That can *strongly* be argued with.
>
> In particular, the use of #ifdef is crap to begin with.  Using #if 
> even for the preprocessor makes it possible to trap misspellings. 

Yes, I'd agree if we were starting from scratch.  But given that we 
can't get rid of CONFIG_* and their dubious semantics, we just have to 
make do.

But typo-detection *would* be very nice: I can never remember if its 
CONFIG_MEMORY_HOTPLUG or CONFIG_HOTPLUG_MEMORY.

    J

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
>>
>> In particular, the use of #ifdef is crap to begin with.  Using #if 
>> even for the preprocessor makes it possible to trap misspellings. 
> 
> Yes, I'd agree if we were starting from scratch.  But given that we 
> can't get rid of CONFIG_* and their dubious semantics, we just have to 
> make do.
> 

That's a very defeatist stance, and quite frankly bogus.

Doing it as a flag day event is not really practical, which is why we 
need a new set of symbols.  However, at that point we can discourage 
continuing use of the CONFIG_ symbols and deprecate them over time. 
It's not like we're talking about user-space-visible interfaces here!

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[Jeremy Fitzhardinge]

H. Peter Anvin wrote:
> That's a very defeatist stance, and quite frankly bogus.

But <stamp foot> coding is *hard*.

> Doing it as a flag day event is not really practical, which is why we 
> need a new set of symbols.  However, at that point we can discourage 
> continuing use of the CONFIG_ symbols and deprecate them over time. 
> It's not like we're talking about user-space-visible interfaces here!

Well, I'm thinking more along the lines of:

   1. We introduce this <whatever> mechanism
   2. Hundreds of people pop out of the woodwork thinking "this looks
      more fun than tweaking whitespace"
   3. They produce one-hundred trillion "convert #ifdef to if()" patches
   4. We have one-hundred trillion^2 followup "fix build with this
      .config" patches

3 might be enough to finally drive Andrew out of the kernel business, 
but 4 definitely would be.

The whole point is to try and get config-invariant build breakages, so 
that we become less dependent on lots of randconfig testing.  If the 
definedness of the KCONFIG_ constants is still dependent on a particular 
.config, then we're not really making all that much progress.

If we move to having a single unified kernel config rather than per-arch 
ones (as Sam mentioned), then we can be sure of generating a complete 
list of all config variables, and we can explicitly define them.  But if 
we don't move to that state more or less simultaneously with using 
KCONFIG_ constants, then we should do it in the defeatest way so we can 
make forward progress with minimal regression.

    J

Re: kernel coding style for if ... else which cross #ifdef

[Adrian Bunk]

On Sat, May 24, 2008 at 10:15:11PM +0100, Jeremy Fitzhardinge wrote:
> H. Peter Anvin wrote:
>> That's a very defeatist stance, and quite frankly bogus.
>
> But <stamp foot> coding is *hard*.
>
>> Doing it as a flag day event is not really practical, which is why we  
>> need a new set of symbols.  However, at that point we can discourage  
>> continuing use of the CONFIG_ symbols and deprecate them over time.  
>> It's not like we're talking about user-space-visible interfaces here!
>
> Well, I'm thinking more along the lines of:
>
>   1. We introduce this <whatever> mechanism
>   2. Hundreds of people pop out of the woodwork thinking "this looks
>      more fun than tweaking whitespace"
>   3. They produce one-hundred trillion "convert #ifdef to if()" patches
>   4. We have one-hundred trillion^2 followup "fix build with this
>      .config" patches
>
> 3 might be enough to finally drive Andrew out of the kernel business,  
> but 4 definitely would be.
>
> The whole point is to try and get config-invariant build breakages, so  
> that we become less dependent on lots of randconfig testing.
>...

We do not have any serious problems where we actually depend on 
randconfig.

randconfig is nice, but even if it would suddenly become no longer 
available we wouldn't have significantely more build breakages in 
stable kernels (perhaps a few more in the pathological cornercases
only randconfig hits).

If this was really "the whole point" it wasn't not worth it.

Real value would come from getting errors for mistyped kconfig variable 
names.

>    J

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Adrian Bunk wrote:
> 
> We do not have any serious problems where we actually depend on 
> randconfig.
> 
> randconfig is nice, but even if it would suddenly become no longer 
> available we wouldn't have significantely more build breakages in 
> stable kernels (perhaps a few more in the pathological cornercases
> only randconfig hits).
> 
> If this was really "the whole point" it wasn't not worth it.
> 
> Real value would come from getting errors for mistyped kconfig variable 
> names.
> 

Indeed.  However, getting at least compiler coverage across all branches 
of code is worth something, at least.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Vegard Nossum wrote:
> 
> Why not use all-lowercase config_* names? It seems elegant, and fits
> in with the notion that these are to be used not as macros, but as
> ordinary constants.
> 
> (The only disadvantage I can see is that they will stand out less. But
> I don't know how great the disadvantage is.)
> 

I think that would be a massive disadvantage.  Furthermore, they *can* 
(and arguably *should*) still be used for the preprocessor; something like:

#if CFG_BLUTTAN

... at least can be configured to warn on a typo.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> I have to say I'm not very keen on the CFG_* prefix.  It doesn't have 
> any inherent meaning and just looks like a redundant abbreviation of 
> CONFIG_

Well, that's pretty much what it was meant to be.

	-hpa

Re: kernel coding style for if ... else which cross #ifdef

[Sam Ravnborg]

On Sat, May 24, 2008 at 04:45:52PM +0100, Jeremy Fitzhardinge wrote:
> Sam Ravnborg wrote:
> >We should actually do as you intially suggested and alwyas
> >define CONFIG_FOO no matter if FOO is built-in or module.
> >Because we do only want to distingush between the two in rare cases.
> >
> >But that is a separate patch and lets not do the same
> >mistage with CFG_*
> >  
> 
> I think pretty strongly that CFG_ and CONFIG_ should be exactly 
> parallel.  If you want to change the meaning of CONFIG_X in the presence 
> of modules, then change CFG_X at the same time.  Making them have 
> different meanings will just confuse anyone wanting to convert #ifdef 
> CONFIG_ code into if(CFG_) code.

We agree they should have the same semantics - we do just not agree on
the timing.
I would love to do a two patch set:
1) Introduce CFG_
2) Alwyas define CONFIG_FOO in case of modules

But I ned someone to audit the use of CONFIG_FOO before I do
such a change. 
I could just do it - but I'm pretty sure it will hurt.
And I do not want to introduce CFG_ with the same
IMO wrong semantic.

> >We may use CFG_* here and there and clash is not good.
And this needs to be checked too - but this is almost trivial to do.

> I have to say I'm not very keen on the CFG_* prefix.  It doesn't have 
> any inherent meaning and just looks like a redundant abbreviation of 
> CONFIG_; something which actually expresses the notion that it's always 
> a compile-time constant would be better.  Not that I have any 
> particularly good alternatives: CONST_? CCONST_? CONFIG_X_VAL? KCONFIG_? 
> KONFIG_? KCONST_?
Of the above I would prefer KCONFIG_FOO if we do not go for
the CFG_FOO version.
That it is const is already given by being UPPERCASE.

	Sam

Re: kernel coding style for if ... else which cross #ifdef

[H. Peter Anvin]

Sam Ravnborg wrote:
>> I don't think we want to use "1 or 2"... I suspect we want to use the 
>> same booleans we currently have.
> I'm a bit dense (or I need more coffe - it's morning here).
> What "same booleans"?

Sorry, we already have CONFIG_FOO (meaning =y) and CONFIG_FOO_MODULE 
(meaning =m).  This seems to work well and will generally do the right 
thing.

If we had CFG_FOO=2 for the case of FOO=n then the clean use of:

	if (CFG_FOO && blah)...

... wouldn't work as nicely.

>> I would suggest CFG_* instead of CONFIG_* for the new set.
> Agreed.

	-hpa