[PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[Masahiro Yamada]

For the same reason as commit 02d699f1f464 ("include/linux/kconfig.h:
ese macros which are already defined"), it is better to use macros
IS_BUILTIN() and IS_MODULE() for defining IS_REACHABLE().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 include/linux/kconfig.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index b33c779..e0182e1 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -41,8 +41,8 @@
  * This is similar to IS_ENABLED(), but returns false when invoked from
  * built-in code when CONFIG_FOO is set to 'm'.
  */
-#define IS_REACHABLE(option) (config_enabled(option) || \
-		 (config_enabled(option##_MODULE) && config_enabled(MODULE)))
+#define IS_REACHABLE(option) (IS_BUILTIN(option) || \
+		 (IS_MODULE(option) && config_enabled(MODULE)))
 
 /*
  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
-- 
1.9.1

Re: [PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[Linus Torvalds]

Side note:

On Mon, Jun 6, 2016 at 2:28 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> -#define IS_REACHABLE(option) (config_enabled(option) || \
> -                (config_enabled(option##_MODULE) && config_enabled(MODULE)))
> +#define IS_REACHABLE(option) (IS_BUILTIN(option) || \
> +                (IS_MODULE(option) && config_enabled(MODULE)))

Is that "config_enabled(MODULE)" actually sensible?

The whole "config_enabled()" thing is designed for config options. But
"MODULE" is not a config option, it's per-file build option ("are we
now building for a module" vs "are we building built-in code").

The code clearly works, but it smells a bit confusing to me. Talking
about "config" of MODULE makes me think CONFIG_MODULES ("are modular
builds enabled") rather than "am I currently building a module".

I wonder if we should have something like

  #ifdef MODULE
   #define BUILDING_MODULES 1
  #else
   #define BUILDING_MODULES 0
  #endif

and then using (IS_MODULE(option) && BUILDING_MODULES) to clarify the test.

Because when I first looked at the patch and didn't think about it any
more, my initial reaction was "why is it checking whether modules are
enabled - if IS_MODULE() is true, then _obviously_ modules are
enabled?"

But maybe that's just me.

               Linus

Re: [PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[Masahiro Yamada]

Hi Linus,


2016-06-07 1:36 GMT+09:00 Linus Torvalds <torvalds@linux-foundation.org>:
> Side note:
>
> On Mon, Jun 6, 2016 at 2:28 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>>
>> -#define IS_REACHABLE(option) (config_enabled(option) || \
>> -                (config_enabled(option##_MODULE) && config_enabled(MODULE)))
>> +#define IS_REACHABLE(option) (IS_BUILTIN(option) || \
>> +                (IS_MODULE(option) && config_enabled(MODULE)))
>
> Is that "config_enabled(MODULE)" actually sensible?
>
> The whole "config_enabled()" thing is designed for config options. But
> "MODULE" is not a config option, it's per-file build option ("are we
> now building for a module" vs "are we building built-in code").

I thought of this, too.

Because config_enabled() is so useful,
maybe people tend to abuse it.


I see one case where config_enabled() is used
for a non-config macro.


#define __EXPORT_SYMBOL(sym, sec) \
           __cond_export_sym(sym, sec, config_enabled(__KSYM_##sym))

Assuming we can do something with that,
ultimately I'd like to ban the use of
config_enabled() outside of include/linux/kconfig.h

I already started this work:
http://www.spinics.net/lists/mips/msg63759.html




> The code clearly works, but it smells a bit confusing to me. Talking
> about "config" of MODULE makes me think CONFIG_MODULES ("are modular
> builds enabled") rather than "am I currently building a module".
>
> I wonder if we should have something like
>
>   #ifdef MODULE
>    #define BUILDING_MODULES 1
>   #else
>    #define BUILDING_MODULES 0
>   #endif
>
> and then using (IS_MODULE(option) && BUILDING_MODULES) to clarify the test.


MODULE is defined / undefined per file.

So, I think BUILDING_MODULE makes more sense than BUILDING_MODULES.



> Because when I first looked at the patch and didn't think about it any
> more, my initial reaction was "why is it checking whether modules are
> enabled - if IS_MODULE() is true, then _obviously_ modules are
> enabled?"
>
> But maybe that's just me.
>
>                Linus



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[Nicolas Pitre]

On Tue, 7 Jun 2016, Masahiro Yamada wrote:

> Because config_enabled() is so useful,
> maybe people tend to abuse it.
> 
> I see one case where config_enabled() is used
> for a non-config macro.
> 
> #define __EXPORT_SYMBOL(sym, sec) \
>            __cond_export_sym(sym, sec, config_enabled(__KSYM_##sym))

Here the need is for a macro that returns 1 or 0 whether given 
symbol is defined or not, exactly as explained in the comment above the 
definition for config_enabled() which in itself has nothing to do with 
config.

So maybe config_enabled() should be renamed to __is_defined() or 
similar, and then config_enabled() or its replacement defined in termps 
of it.


Nicolas

Re: [PATCH] kconfig.h: use already defined macros for IS_REACHABLE() define

[Masahiro Yamada]

Hi Nicolas,

2016-06-07 7:03 GMT+09:00 Nicolas Pitre <nicolas.pitre@linaro.org>:
> On Tue, 7 Jun 2016, Masahiro Yamada wrote:
>
>> Because config_enabled() is so useful,
>> maybe people tend to abuse it.
>>
>> I see one case where config_enabled() is used
>> for a non-config macro.
>>
>> #define __EXPORT_SYMBOL(sym, sec) \
>>            __cond_export_sym(sym, sec, config_enabled(__KSYM_##sym))
>
> Here the need is for a macro that returns 1 or 0 whether given
> symbol is defined or not, exactly as explained in the comment above the
> definition for config_enabled() which in itself has nothing to do with
> config.
>
> So maybe config_enabled() should be renamed to __is_defined() or
> similar, and then config_enabled() or its replacement defined in termps
> of it.

__is_defined() seems reasonable to me, so I've sent an updated series.





-- 
Best Regards
Masahiro Yamada