Re: [Linux-eng] [RFC 0/3] Abstract empty functions with STUB_UNLESS macro

[Masahiro Yamada]

On Sat, Jan 19, 2019 at 1:02 AM Andrew Murray <andrew.murray@xxxxxxx> wrote:
>
> A common pattern found in header files is a function declaration dependent
> on a CONFIG_ option being enabled, followed by an empty function for when
> that option isn't enabled. This boilerplate code can often take up a lot
> of space and impact code readability.
>
> This series introduces a STUB_UNLESS macro that simplifies header files as
> follows:
>
> STUB_UNLESS(CONFIG_FOO, [body], prototype)
>
> This evaluates to 'prototype' prepended with 'extern' if CONFIG_FOO is set
> to 'y'. Otherwise it will evaluate to 'prototype' prepended with 'static
> inline' followed by an empty function body. Where optional argument 'body'
> is present then 'body' will be used as the function body, intended to allow
> simple return statements. Using the macro results in hunks such as this:
>
> -#ifdef CONFIG_HAVE_HW_BREAKPOINT
> -extern void hw_breakpoint_thread_switch(struct task_struct *next);
> -extern void ptrace_hw_copy_thread(struct task_struct *task);
> -#else
> -static inline void hw_breakpoint_thread_switch(struct task_struct *next)
> -{
> -}
> -static inline void ptrace_hw_copy_thread(struct task_struct *task)
> -{
> -}
> -#endif
> +STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT,
> +void hw_breakpoint_thread_switch(struct task_struct *next));
> +
> +STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT,
> +void ptrace_hw_copy_thread(struct task_struct *task));
>
> This may or may not be considered as more desirable than the status quo.
>
> This series updates arm64 and perf to use the macro as an example.
>
> Andrew Murray (3):
>   kconfig.h: abstract empty functions with STUB_UNLESS macro
>   cpufreq: update headers to use STUB_UNLESS macro
>   arm64: update headers to use STUB_UNLESS macro
>
>  arch/arm64/include/asm/acpi.h           | 38 +++++++++-------------
>  arch/arm64/include/asm/alternative.h    |  6 +---
>  arch/arm64/include/asm/cpufeature.h     |  6 +---
>  arch/arm64/include/asm/cpuidle.h        | 18 +++--------
>  arch/arm64/include/asm/debug-monitors.h | 10 ++----
>  arch/arm64/include/asm/fpsimd.h         | 57 +++++++++++++--------------------
>  arch/arm64/include/asm/hw_breakpoint.h  | 16 +++------
>  arch/arm64/include/asm/kasan.h          |  9 ++----
>  arch/arm64/include/asm/numa.h           | 19 ++++-------
>  arch/arm64/include/asm/ptdump.h         | 13 +++-----
>  arch/arm64/include/asm/signal32.h       | 29 +++++------------
>  include/linux/cpufreq.h                 | 21 ++++--------
>  include/linux/kconfig.h                 | 31 ++++++++++++++++++
>  13 files changed, 110 insertions(+), 163 deletions(-)
>
> --
> 2.7.4
>

Honestly, I am not a big fan of shorting the code
for the purpose of shorting.


This patch series is based on the assumption
the first argument is "defined or undefined".
In other words, it cannot handle tristate CONFIG option.


But, if you go this way, could you make
it work in a more generic manner?

And, decouple this from <linux/kconfig.h>



For example, see the following cases:

https://github.com/torvalds/linux/blob/v5.0-rc2/include/acpi/button.h#L7

https://github.com/torvalds/linux/blob/v5.0-rc2/include/linux/firmware.h#L42




The latter is a good example.

In many cases, the kernel headers look like this:


#ifdef CONFIG_FOO

  { large block of prototype declaration }

#else

  { large block of nop stubs }

#endif


So, this approach shifts "duplication of prototype"
into "duplication of conditional part".


-- 
Best Regards
Masahiro Yamada