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

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