| Message ID | 1305646532-29114-1-git-send-email-mmarek@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17:35 Tue 17 May , Michal Marek wrote: > For strings and integers, the config_is_xxx macros are useless and > sometimes misleading: except if the interger or hex can be at 0 so the config_is_ is usefull to known that it's enabled > > #define CONFIG_INITRAMFS_SOURCE "" > #define config_is_initramfs_source() 1 here agreed but I'm nor sure if it's a special case or if it will we the case for most of the string Best Regards, J. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17.5.2011 20:05, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 17:35 Tue 17 May , Michal Marek wrote: >> For strings and integers, the config_is_xxx macros are useless and >> sometimes misleading: > except if the interger or hex can be at 0 > so the config_is_ is usefull to known that it's enabled You can check if the option that the integer depends on is enabled. >> #define CONFIG_INITRAMFS_SOURCE "" >> #define config_is_initramfs_source() 1 > here agreed but I'm nor sure if it's a special case or if it will we the case > for most of the string Same here. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: > For strings and integers, the config_is_xxx macros are useless and > sometimes misleading: > > #define CONFIG_INITRAMFS_SOURCE "" > #define config_is_initramfs_source() 1 I'm late with this comment.... Could we introduce "config_is_foo" using a syntax that does not break grepability? Maybe a syntax like this? #ifdef CONFIG_FOO and if (KCONFIG_FOO()) Grepping for the use of a symbol is a very typical thing, so we should try to keep this. And with the suggested syntax above I expect fixdep to catch up both usage types. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, [added Valdis.Kletnieks@vt.edu to CC:] On Tue, May 17, 2011 at 3:53 PM, Sam Ravnborg <sam@ravnborg.org> wrote: > On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: >> For strings and integers, the config_is_xxx macros are useless and >> sometimes misleading: >> >> #define CONFIG_INITRAMFS_SOURCE "" >> #define config_is_initramfs_source() 1 > > I'm late with this comment.... > Could we introduce "config_is_foo" using a syntax that > does not break grepability? > > Maybe a syntax like this? > > #ifdef CONFIG_FOO > > and > > if (KCONFIG_FOO()) > > Grepping for the use of a symbol is a very typical thing, > so we should try to keep this. > And with the suggested syntax above I expect fixdep to > catch up both usage types. > Actually, there is already an issue, on a much smaller scale, in the current tree with NUMA_BUILD and COMPACTION_BUILD. The real way to fix this would be to always defines CONFIG_FOO, its value being 1 or 0 depending on whether or not the symbol is selected. This is a +35k/-35k change. Also, I find KCONFIG_FOO() is too specific to be in the kernel source, and redundant with CONFIG_FOO. I've been playing a bit with the preprocessor, and reached that point: #define EXPAND(x) __ ## x #define CONFIGURED(x) \ ({ int __1 __maybe_unused = 1; \ int __ ## x __maybe_unused = 0; \ EXPAND(x); }) I am not specifically proud of that, use case would be: extern func(void); int fn() { if(CONFIGURED(CONFIG_FOO)) func(); } The issue I am seeing is that it adds a dependency to the optimizer, as gcc will not optimize the branch away at -O0. The advantage is that it is not intrusive at all within kconfig. - Arnaud -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, May 18, 2011 at 2:19 AM, Sam Ravnborg <sam@ravnborg.org> wrote: > On Wed, May 18, 2011 at 07:16:45AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On 21:53 Tue 17 May , Sam Ravnborg wrote: >> > On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: >> > > For strings and integers, the config_is_xxx macros are useless and >> > > sometimes misleading: >> > > >> > > #define CONFIG_INITRAMFS_SOURCE "" >> > > #define config_is_initramfs_source() 1 >> > >> > I'm late with this comment.... >> > Could we introduce "config_is_foo" using a syntax that >> > does not break grepability? >> > >> > Maybe a syntax like this? >> > >> > #ifdef CONFIG_FOO >> > >> > and >> > >> > if (KCONFIG_FOO()) >> > >> > Grepping for the use of a symbol is a very typical thing, >> > so we should try to keep this. >> > And with the suggested syntax above I expect fixdep to >> > catch up both usage types. >> I'll prefer kconfig_foo() >> not uppercase >> >> but if we use KCONFIG_FOO no need to touch fixdep > > fixdep is easy to fix - albeit it may cost a bit of processing time. > wht I worry much more about is users that miss uses of CONFIG_ symbols, because > they do not show up in "git grep CONFIG_FOO". > agree. > Using "if (KCONFIG_FOO()) users are also awre this is a configuration > decided condition - which is nice to stand out. > this will not work if you do $(git -w CONFIG_FOO) to avoid getting all kind of noise in your search. - Arnaud -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: > For strings and integers, the config_is_xxx macros are useless and > sometimes misleading: > > #define CONFIG_INITRAMFS_SOURCE "" > #define config_is_initramfs_source() 1 > > Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > Signed-off-by: Michal Marek <mmarek@suse.cz> > --- > scripts/kconfig/confdata.c | 8 +++----- > 1 files changed, 3 insertions(+), 5 deletions(-) I applied this to kbuild-2.6.git#kconfig, but I won't be sending this and the config_is_xxx patch to Linus now. First, I'm a bit in hurry and there have been proposals for either a different name of the macro, or a different implementation, second, the fixdep fix is not here, so we wouldn't be able to make use of the feature anyway. So let's settle on a final solution for 2.6.41 or 3.1 or whatever comes second next. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18.5.2011 08:23, Arnaud Lacombe wrote: > Hi, > > [added Valdis.Kletnieks@vt.edu to CC:] > > On Tue, May 17, 2011 at 3:53 PM, Sam Ravnborg<sam@ravnborg.org> wrote: >> On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: >>> For strings and integers, the config_is_xxx macros are useless and >>> sometimes misleading: >>> >>> #define CONFIG_INITRAMFS_SOURCE "" >>> #define config_is_initramfs_source() 1 >> >> I'm late with this comment.... >> Could we introduce "config_is_foo" using a syntax that >> does not break grepability? >> >> Maybe a syntax like this? >> >> #ifdef CONFIG_FOO >> >> and >> >> if (KCONFIG_FOO()) >> >> Grepping for the use of a symbol is a very typical thing, >> so we should try to keep this. >> And with the suggested syntax above I expect fixdep to >> catch up both usage types. >> > Actually, there is already an issue, on a much smaller scale, in the > current tree with NUMA_BUILD and COMPACTION_BUILD. The real way to fix > this would be to always defines CONFIG_FOO, its value being 1 or 0 > depending on whether or not the symbol is selected. This is a > +35k/-35k change. > > Also, I find KCONFIG_FOO() is too specific to be in the kernel source, > and redundant with CONFIG_FOO. > > I've been playing a bit with the preprocessor, and reached that point: > > #define EXPAND(x) __ ## x > #define CONFIGURED(x) \ > ({ int __1 __maybe_unused = 1; \ > int __ ## x __maybe_unused = 0; \ > EXPAND(x); }) > > I am not specifically proud of that, use case would be: > > extern func(void); > int fn() > { > if(CONFIGURED(CONFIG_FOO)) > func(); > } I finally got round to revisit this. Your approach inspired me to a much simpler scheme: Instead of generating the config_is_xxx macros for direct use in the code, name them __enabled_CONFIG_XXX or similar and have a macro that expands given CONFIG_XXX symbol to the other form: /* * Usage: ENABLED(CONFIG_FOO) * Please do not use the __enabled_CONFIG_FOO defines directly to not break * grepability of the code. */ #define ENABLED(x) __enabled_ ## x plus a checkpatch.pl check so that people do not use the __enabled_CONFIG_FOO macros in their code. git grep -w CONFIG_FOO continues to work, fixdep continues to work, it works with -O0 because it expands to a if(1) or if(0). Am I missing some obvious problem? Attached is my test program: $ gcc -Wall -O0 test.c $ ./a.out Foo1.0 Foo1.1 $ strings ./a.out | grep Foo Foo1.0 Foo1.1 Michal #include <stdio.h> #define CONFIG_FOO1 1 #undef CONFIG_FOO2 #define __enabled_CONFIG_FOO1 1 #define __enabled_CONFIG_FOO2 0 /* * Usage: ENABLED(CONFIG_FOO) * Please do not use the __enabled_CONFIG_FOO defines directly to not break * grepability of the code. */ #define ENABLED(x) __enabled_ ## x int main(void) { #ifdef CONFIG_FOO1 puts("Foo1.0"); #endif #ifdef CONFIG_FOO2 puts("Foo2.0"); #endif if (ENABLED(CONFIG_FOO1)) { puts("Foo1.1"); } if (ENABLED(CONFIG_FOO2)) { puts("Foo2.1"); } return 0; }
Hi, On Wed, Jul 13, 2011 at 9:22 AM, Michal Marek <mmarek@suse.cz> wrote: > On 18.5.2011 08:23, Arnaud Lacombe wrote: >> >> Hi, >> >> [added Valdis.Kletnieks@vt.edu to CC:] >> >> On Tue, May 17, 2011 at 3:53 PM, Sam Ravnborg<sam@ravnborg.org> wrote: >>> >>> On Tue, May 17, 2011 at 05:35:32PM +0200, Michal Marek wrote: >>>> >>>> For strings and integers, the config_is_xxx macros are useless and >>>> sometimes misleading: >>>> >>>> #define CONFIG_INITRAMFS_SOURCE "" >>>> #define config_is_initramfs_source() 1 >>> >>> I'm late with this comment.... >>> Could we introduce "config_is_foo" using a syntax that >>> does not break grepability? >>> >>> Maybe a syntax like this? >>> >>> #ifdef CONFIG_FOO >>> >>> and >>> >>> if (KCONFIG_FOO()) >>> >>> Grepping for the use of a symbol is a very typical thing, >>> so we should try to keep this. >>> And with the suggested syntax above I expect fixdep to >>> catch up both usage types. >>> >> Actually, there is already an issue, on a much smaller scale, in the >> current tree with NUMA_BUILD and COMPACTION_BUILD. The real way to fix >> this would be to always defines CONFIG_FOO, its value being 1 or 0 >> depending on whether or not the symbol is selected. This is a >> +35k/-35k change. >> >> Also, I find KCONFIG_FOO() is too specific to be in the kernel source, >> and redundant with CONFIG_FOO. >> >> I've been playing a bit with the preprocessor, and reached that point: >> >> #define EXPAND(x) __ ## x >> #define CONFIGURED(x) \ >> ({ int __1 __maybe_unused = 1; \ >> int __ ## x __maybe_unused = 0; \ >> EXPAND(x); }) >> >> I am not specifically proud of that, use case would be: >> >> extern func(void); >> int fn() >> { >> if(CONFIGURED(CONFIG_FOO)) >> func(); >> } > > I finally got round to revisit this. Your approach inspired me to a much > simpler scheme: Instead of generating the config_is_xxx macros for direct > use in the code, name them __enabled_CONFIG_XXX or similar and have a macro > that expands given CONFIG_XXX symbol to the other form: > > /* > * Usage: ENABLED(CONFIG_FOO) > * Please do not use the __enabled_CONFIG_FOO defines directly to not break > * grepability of the code. > */ > #define ENABLED(x) __enabled_ ## x > > plus a checkpatch.pl check so that people do not use the > __enabled_CONFIG_FOO macros in their code. git grep -w CONFIG_FOO continues > to work, fixdep continues to work, it works with -O0 because it expands to a > if(1) or if(0). Am I missing some obvious problem? > not I see immediately. ENABLED() will conflict with existing keyword in the tree, so it might need tweaking. Basically, you are taking the approach of always defining CONFIG_FOO (to 0 or 1), but by introducing a new macro, you avoid to break #ifdef usage in the tree. Actually, with this approach, we can even see forward and start replacing: #ifdef CONFIG_FOO by #if ENABLED(CONFIG_FOO) In a couple of release, we mark the old #ifdef syntax as deprecated, then after a couple of release, get rid of the duplicated macro altogether. You evil ! :-) - Arnaud > Attached is my test program: > $ gcc -Wall -O0 test.c > $ ./a.out > Foo1.0 > Foo1.1 > $ strings ./a.out | grep Foo > Foo1.0 > Foo1.1 > > Michal > -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index db06af0..d40195d 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -808,7 +808,6 @@ int conf_write_autoconf(void) const char *name; FILE *out, *tristate, *out_h; int i; - int fct_val; sym_clear_all_valid(); @@ -849,7 +848,7 @@ int conf_write_autoconf(void) rootmenu.prompt->text); for_all_symbols(i, sym) { - fct_val = 1; + int fct_val = 0; sym_calc_value(sym); if (!(sym->flags & SYMBOL_WRITE) || !sym->name) continue; @@ -863,7 +862,6 @@ int conf_write_autoconf(void) case S_TRISTATE: switch (sym_get_tristate_value(sym)) { case no: - fct_val = 0; break; case mod: fprintf(tristate, "%s%s=M\n", @@ -878,8 +876,10 @@ int conf_write_autoconf(void) CONFIG_, sym->name); fprintf(out_h, "#define %s%s 1\n", CONFIG_, sym->name); + fct_val = 1; break; } + conf_write_function_autoconf(out_h, CONFIG_, sym->name, fct_val); break; case S_STRING: conf_write_string(true, sym->name, sym_get_string_value(sym), out_h); @@ -897,10 +897,8 @@ int conf_write_autoconf(void) CONFIG_, sym->name, str); break; default: - fct_val = 0; break; } - conf_write_function_autoconf(out_h, CONFIG_, sym->name, fct_val); } fclose(out); fclose(tristate);
For strings and integers, the config_is_xxx macros are useless and sometimes misleading: #define CONFIG_INITRAMFS_SOURCE "" #define config_is_initramfs_source() 1 Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> Signed-off-by: Michal Marek <mmarek@suse.cz> --- scripts/kconfig/confdata.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)