[PATCH v2 2/3] doc/develop/codingstyle.rst: Add a section on conditional compilation

[Tom Rini]

On Thu, Mar 13, 2025 at 11:15:35AM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 3/12/25 2:00 AM, Tom Rini wrote:
> > In order to make a start on explaining how and when to use certain
> > macros, we need to document their usage somewhere. As a first step, take
> > section 21 of the v6.13 Linux Kernel coding-style document on
> > conditional compilation and add it here.
> > 
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> > ---
> > Changes in v2:
> > - New patch.
> > ---
> >   doc/develop/codingstyle.rst | 48 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 48 insertions(+)
> > 
> > diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
> > index fa3cd6aec82e..7211e4e4eed1 100644
> > --- a/doc/develop/codingstyle.rst
> > +++ b/doc/develop/codingstyle.rst
> > @@ -154,6 +154,54 @@ See `here
> >   <https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation>`_
> >   for style.
> > +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:
> > +
> > +.. code-block:: c
> > +
> > +	if (IS_ENABLED(CONFIG_SOMETHING)) {
> > +		...
> > +	}
> > +
> 
> We should mentiond CONFIG_IS_ENABLED here too, as this is probably what most
> people should be using in driver code?
> 
> > +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.
> > +
> 
> And I think this is a perfect example why we should follow the
> aforementioned best practice as this would not happen had we had a stub
> version of the function instead of the function just not existing? It'd be
> nice to give that as an example in the docs. Follow-up question: is there a
> reason for NOT wanting to do that?
> 
> > +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:
> > +
> > +.. code-block:: c
> > +
> > +	#ifdef CONFIG_SOMETHING
> > +	...
> > +	#endif /* CONFIG_SOMETHING */
> > +
> 
> Applicable to the whole patch, maybe highlight non-English words with double
> tick quotes?
> 
> e.g. ``#if``, ``#ifdef``, ``.c``, ``#else``, ``__maybe_unused``,
> ``IS_ENABLED``, etc...
> 
> Looks good to me otherwise, nice addition!

I'll reword the commit message to be clearer that this is just literally
copy / pasting the kernel section as a starting point.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250313/2d233ee5/attachment.sig>