[PATCH] CodingStyle: Add a chapter on conditional compilation

[PATCH] CodingStyle: Add a chapter on conditional compilation

[Josh Triplett]

Document several common practices and conventions regarding conditional
compilation, most notably the preference for ifdefs in headers rather
than .c files.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---

I found myself explaining a few of these unwritten rules in patch
feedback, so I figured I'd document them.

 Documentation/CodingStyle | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 3171822..9f28b14 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -845,6 +845,49 @@ next instruction in the assembly output:
 	     : /* outputs */ : /* inputs */ : /* clobbers */);
 
 
+		Chapter 20: Conditional Compilation
+
+Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
+files; doing so makes code harder to read and logic harder to follow.  Instead,
+use such conditionals in a header file defining functions for use in those .c
+files, providing no-op stub versions in the #else case, and then call those
+functions unconditionally from .c files.  The compiler will avoid generating
+any code for the stub calls, producing identical results, but the logic will
+remain easy to follow.
+
+Prefer to compile out entire functions, rather than portions of functions or
+portions of expressions.  Rather than putting an ifdef in an expression, factor
+out part or all of the expression into a separate helper function and apply the
+conditional to that function.
+
+If you have a function or variable which may potentially go unused in a
+particular configuration, and the compiler would warn about its definition
+going unused, mark the definition as __maybe_unused rather than wrapping it in
+a preprocessor conditional.  (However, if a function or variable *always* goes
+unused, delete it.)
+
+Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
+symbol into a C boolean expression, and use it in a normal C conditional:
+
+	if (IS_ENABLED(CONFIG_SOMETHING)) {
+		...
+	}
+
+The compiler will constant-fold the conditional away, and include or exclude
+the block of code just as with an #ifdef, so this will not add any runtime
+overhead.  However, this approach still allows the C compiler to see the code
+inside the block, and check it for correctness (syntax, types, symbol
+references, etc).  Thus, you still have to use an #ifdef if the code inside the
+block references symbols that will not exist if the condition is not met.
+
+At the end of any non-trivial #if or #ifdef block (more than a few lines),
+place a comment after the #endif on the same line, noting the conditional
+expression used.  For instance:
+
+#ifdef CONFIG_SOMETHING
+...
+#endif /* CONFIG_SOMETHING */
+
 
 		Appendix I: References
 
-- 
2.1.1

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Geert Uytterhoeven]

On Wed, Oct 29, 2014 at 7:15 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> Document several common practices and conventions regarding conditional
> compilation, most notably the preference for ifdefs in headers rather
> than .c files.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

> +If you have a function or variable which may potentially go unused in a
> +particular configuration, and the compiler would warn about its definition
> +going unused, mark the definition as __maybe_unused rather than wrapping it in
> +a preprocessor conditional.  (However, if a function or variable *always* goes
> +unused, delete it.)

Personally, I don't like __maybe_unused. Once it's there, the compiler
will stop warning about it, even if it really becomes unused.

Apart from that:
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Josh Triplett]

On Wed, Oct 29, 2014 at 08:12:49PM +0100, Geert Uytterhoeven wrote:
> On Wed, Oct 29, 2014 at 7:15 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > Document several common practices and conventions regarding conditional
> > compilation, most notably the preference for ifdefs in headers rather
> > than .c files.
> >
> > Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> 
> > +If you have a function or variable which may potentially go unused in a
> > +particular configuration, and the compiler would warn about its definition
> > +going unused, mark the definition as __maybe_unused rather than wrapping it in
> > +a preprocessor conditional.  (However, if a function or variable *always* goes
> > +unused, delete it.)
> 
> Personally, I don't like __maybe_unused. Once it's there, the compiler
> will stop warning about it, even if it really becomes unused.

True.  It's a tradeoff between getting the compiler to warn about unused
code to allow removing it, and putting #ifdefs in .c files.  However,
in previous patch discussions, developers seem to come down pretty
consistently on the side of "don't put #ifdefs in .c files".

> Apart from that:
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks!

- Josh Triplett

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Randy Dunlap]

On 10/29/14 12:12, Geert Uytterhoeven wrote:
> On Wed, Oct 29, 2014 at 7:15 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> Document several common practices and conventions regarding conditional
>> compilation, most notably the preference for ifdefs in headers rather
>> than .c files.
>>
>> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> 
>> +If you have a function or variable which may potentially go unused in a
>> +particular configuration, and the compiler would warn about its definition
>> +going unused, mark the definition as __maybe_unused rather than wrapping it in
>> +a preprocessor conditional.  (However, if a function or variable *always* goes
>> +unused, delete it.)
> 
> Personally, I don't like __maybe_unused. Once it's there, the compiler
> will stop warning about it, even if it really becomes unused.
> 
> Apart from that:
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Is the compiler smart enough to delete (discard) the code or data instance
if it is unused or is the code or data actually wasting space?


-- 
~Randy

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Josh Triplett]

On Wed, Oct 29, 2014 at 05:35:02PM -0700, Randy Dunlap wrote:
> On 10/29/14 12:12, Geert Uytterhoeven wrote:
> > On Wed, Oct 29, 2014 at 7:15 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> >> Document several common practices and conventions regarding conditional
> >> compilation, most notably the preference for ifdefs in headers rather
> >> than .c files.
> >>
> >> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> > 
> >> +If you have a function or variable which may potentially go unused in a
> >> +particular configuration, and the compiler would warn about its definition
> >> +going unused, mark the definition as __maybe_unused rather than wrapping it in
> >> +a preprocessor conditional.  (However, if a function or variable *always* goes
> >> +unused, delete it.)
> > 
> > Personally, I don't like __maybe_unused. Once it's there, the compiler
> > will stop warning about it, even if it really becomes unused.
> > 
> > Apart from that:
> > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Is the compiler smart enough to delete (discard) the code or data instance
> if it is unused or is the code or data actually wasting space?

If you mark a function or variable as __maybe_unused, and it actually
goes unused, the compiler will silently discard it.

- Josh Triplett

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Martin Kelly]

On 10/29/2014 11:15 AM, Josh Triplett wrote:
> Document several common practices and conventions regarding conditional
> compilation, most notably the preference for ifdefs in headers rather
> than .c files.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
> 
> I found myself explaining a few of these unwritten rules in patch
> feedback, so I figured I'd document them.
> 
>  Documentation/CodingStyle | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 3171822..9f28b14 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -845,6 +845,49 @@ next instruction in the assembly output:
>  	     : /* outputs */ : /* inputs */ : /* clobbers */);
>  
>  
> +		Chapter 20: Conditional Compilation
> +
> +Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> +files; doing so makes code harder to read and logic harder to follow.  Instead,
> +use such conditionals in a header file defining functions for use in those .c
> +files, providing no-op stub versions in the #else case, and then call those
> +functions unconditionally from .c files.  The compiler will avoid generating
> +any code for the stub calls, producing identical results, but the logic will
> +remain easy to follow.
> +
> +Prefer to compile out entire functions, rather than portions of functions or
> +portions of expressions.  Rather than putting an ifdef in an expression, factor
> +out part or all of the expression into a separate helper function and apply the
> +conditional to that function.
> +
> +If you have a function or variable which may potentially go unused in a
> +particular configuration, and the compiler would warn about its definition
> +going unused, mark the definition as __maybe_unused rather than wrapping it in
> +a preprocessor conditional.  (However, if a function or variable *always* goes
> +unused, delete it.)
> +
> +Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
> +symbol into a C boolean expression, and use it in a normal C conditional:
> +
> +	if (IS_ENABLED(CONFIG_SOMETHING)) {
> +		...
> +	}
> +
> +The compiler will constant-fold the conditional away, and include or exclude
> +the block of code just as with an #ifdef, so this will not add any runtime
> +overhead.  However, this approach still allows the C compiler to see the code
> +inside the block, and check it for correctness (syntax, types, symbol
> +references, etc).  Thus, you still have to use an #ifdef if the code inside the
> +block references symbols that will not exist if the condition is not met.
> +
> +At the end of any non-trivial #if or #ifdef block (more than a few lines),
> +place a comment after the #endif on the same line, noting the conditional
> +expression used.  For instance:
> +
> +#ifdef CONFIG_SOMETHING
> +...
> +#endif /* CONFIG_SOMETHING */
> +
>  
>  		Appendix I: References
>  
> 

I like that you're formalizing this in the official document; thanks!

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Jonathan Corbet]

On Wed, 29 Oct 2014 11:15:17 -0700
Josh Triplett <josh@joshtriplett.org> wrote:

> Document several common practices and conventions regarding conditional
> compilation, most notably the preference for ifdefs in headers rather
> than .c files.

OK, I've picked this one up for my 3.19 docs pull.

Thanks,

jon

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Joe Perches]

On Mon, 2014-11-03 at 11:46 -0500, Jonathan Corbet wrote:
> On Wed, 29 Oct 2014 11:15:17 -0700
> Josh Triplett <josh@joshtriplett.org> wrote:
> 
> > Document several common practices and conventions regarding conditional
> > compilation, most notably the preference for ifdefs in headers rather
> > than .c files.
> 
> OK, I've picked this one up for my 3.19 docs pull.

I think that Al Viro's suggestion from awhile ago:

https://lkml.org/lkml/2013/3/20/388

could still be in CodingStyle somewhere or in
another document like CodingStyleSuggestions.

Another thing that could go is the suggestion to
use Lindent.

https://lkml.org/lkml/2013/2/11/390

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Josh Triplett]

On Mon, Nov 03, 2014 at 09:47:40AM -0800, Joe Perches wrote:
> On Mon, 2014-11-03 at 11:46 -0500, Jonathan Corbet wrote:
> > On Wed, 29 Oct 2014 11:15:17 -0700
> > Josh Triplett <josh@joshtriplett.org> wrote:
> > 
> > > Document several common practices and conventions regarding conditional
> > > compilation, most notably the preference for ifdefs in headers rather
> > > than .c files.
> > 
> > OK, I've picked this one up for my 3.19 docs pull.
> 
> I think that Al Viro's suggestion from awhile ago:
> 
> https://lkml.org/lkml/2013/3/20/388
> 
> could still be in CodingStyle somewhere or in
> another document like CodingStyleSuggestions.

I think that text needs some cleanup to better fit CodingStyle, but the
intent and recommendations definitely ought to go in.  A few of those
seem too far down the road of "don't stuff beans up your nose", and some
of them need shortening (just "don't put an else after an if condition
ending with break or return; remember to handle errors via break,
return, or continue, and outdent the subsequent code").

> Another thing that could go is the suggestion to
> use Lindent.
> 
> https://lkml.org/lkml/2013/2/11/390

Agreed completely.  We might consider coming up with settings for
clang-format, which seems like a far more capable replacement that
actually understands C.

- Josh Triplett

Re: [PATCH] CodingStyle: Add a chapter on conditional compilation

[Joe Perches]

On Mon, 2014-11-03 at 10:05 -0800, Josh Triplett wrote:
> On Mon, Nov 03, 2014 at 09:47:40AM -0800, Joe Perches wrote:
[]
> "don't put an else after an if condition
> ending with break or return;

Maybe that's a bit _too_ simple.
Yes for a simple if, but not necessarily an else if

	if (foo) {
		...;
	} else if (bar) {
		more code...;
		return;
	} else {
		...
	}

still needs that last else.

checkpatch isn't very good at that code flow
interpretation so it emits a warning anyway.

> > Another thing that could go is the suggestion to
> > use Lindent.
> > 
> > https://lkml.org/lkml/2013/2/11/390
> 
> Agreed completely.  We might consider coming up with settings for
> clang-format, which seems like a far more capable replacement that
> actually understands C.

Never used it.
It does seem to have a large number of options
(including a trivial linux-kernel style) though.

http://clang.llvm.org/docs/ClangFormatStyleOptions.html

Dunno how much work it would be to create a proper
linux-kernel style for clang-format.

It might work well, it might not.
Anyone have experience with it?