Message ID | 1465205305-31202-1-git-send-email-yamada.masahiro@socionext.com |
---|---|
State | Superseded |
Headers | show |
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
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
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
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',
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(-) -- 1.9.1