| Message ID | 1547827230-55132-1-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Abstract empty functions with STUB_UNLESS macro | expand |
On Fri, Jan 18, 2019 at 04:00:27PM +0000, Andrew Murray 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) Can you explain the desire to make the second argument optional, rather than having the mandatory arguments first and the optional body last? It will mean more lines at each site, but I don't think that's a bad thing: STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT, void hw_breakpoint_thread_switch(struct task_struct *next)); STUB_UNLESS(CONFIG_CPU_FREQ, struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), return NULL); or: STUB_UNLESS(CONFIG_CPU_FREQ, struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), return NULL); Seems to be more readable in terms of the flow.
On Fri, Jan 18, 2019 at 04:37:36PM +0000, Russell King - ARM Linux admin wrote: > On Fri, Jan 18, 2019 at 04:00:27PM +0000, Andrew Murray 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) > > Can you explain the desire to make the second argument optional, > rather than having the mandatory arguments first and the optional body > last? It will mean more lines at each site, but I don't think that's > a bad thing: > > STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT, > void hw_breakpoint_thread_switch(struct task_struct *next)); > > STUB_UNLESS(CONFIG_CPU_FREQ, > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), return NULL); > > or: > > STUB_UNLESS(CONFIG_CPU_FREQ, > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), > return NULL); > > Seems to be more readable in terms of the flow. Hmmm, looking at that, I probably prefer that too. In the unlikely case that <body> uses the function arguments it would be quite confusing to have the body before the function prototype. If we can keep this down to two lines so much the better, but still seems fine. Provided we don't end up needing a trailing comma in the void case, to supply the empty body argument, that is. Cheers ---Dave
On Fri, Jan 18, 2019 at 04:44:25PM +0000, Dave Martin wrote: > On Fri, Jan 18, 2019 at 04:37:36PM +0000, Russell King - ARM Linux admin wrote: > > On Fri, Jan 18, 2019 at 04:00:27PM +0000, Andrew Murray 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) > > > > Can you explain the desire to make the second argument optional, > > rather than having the mandatory arguments first and the optional body > > last? It will mean more lines at each site, but I don't think that's > > a bad thing: My intent was to make the function prototype look like an ordinary prototype but with all this special macro stuff on the preceding line. Much like this: > > > > STUB_UNLESS(CONFIG_HAVE_HW_BREAKPOINT, > > void hw_breakpoint_thread_switch(struct task_struct *next)); Besides the extra ')' at the end it looks like a normal prototype. I felt this may be important as existing tooling (ctags etc) might have a better chance of recognising it and it wouldn't be so alien to new developers. I feared that if the 'prototype' argument was in the middle then it would get lost in all the other arguments and be less readable as a prototype. > > > > STUB_UNLESS(CONFIG_CPU_FREQ, > > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), return NULL); > > > > or: > > > > STUB_UNLESS(CONFIG_CPU_FREQ, > > struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu), > > return NULL); As you indicate here, it's possible to spread this to three lines and keep the readability of the prototype - though I was keen to condense it to as few lines as possible (I was probably putting too much focus on the diff stat). > > > > Seems to be more readable in terms of the flow. > > Hmmm, looking at that, I probably prefer that too. Feedback I've had so far suggests that there is a preference to putting the optional argument at the end, I have no objection to this. > > In the unlikely case that <body> uses the function arguments it would be > quite confusing to have the body before the function prototype. > > If we can keep this down to two lines so much the better, but still > seems fine. This is the compromise - having the optional argument after the prototype will likely result in wrapping to the next line as prototypes tend to be long. Perhaps this is more readable. > > Provided we don't end up needing a trailing comma in the void case, to > supply the empty body argument, that is. No this isn't necessary. Thanks, Andrew Murray > > Cheers > ---Dave
On Sat, Jan 19, 2019 at 1:02 AM Andrew Murray <andrew.murray@arm.com> 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".
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(-)