| Message ID | 1304658229-30820-1-git-send-email-plagnioj@jcrosoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12:19 Fri 06 May , Arnaud Lacombe wrote: > Hi, > > On Fri, May 6, 2011 at 1:03 AM, Jean-Christophe PLAGNIOL-VILLARD > <plagnioj@jcrosoft.com> wrote: > > this will allow to use to use > > > > if(config_is_xxx()) > > if(config_is_xxx_module()) > > > > in the code instead of > > > > #ifdef CONFIG_xxx > > #ifdef CONFIG_xxx_MODULE > > > > and now let the compiler remove the non usefull code and not the > > pre-processor > > > Why would it be a good thing ? > > Most configuration-dependent code inside functions tends to be moved > to a static inline already, which get conditionally defined based on > the CONFIG_<foo>. If it is not, then the code is badly architectured > (-> bad). Using that if(xxx) notation would also lead to yet more > heavily indented function (-> bad). Moreover, this introduces > yet-another way to check for an information (-> bad), and you will end > up with mixing the config_is_<xxx> notation inside a function > declaration, and CONFIG_<xxx> when not inside a function (-> bad) > > Actually, this is even worse than that as you'll not be able to hide > structure (or structure members) inside CONFIG_<xxx> and use that > structure (or structure members) in config_is_<xxx> protected block > without causing compile-time failure. sorry but conditionnal structure members is bad practice you save nearly no space nut for the test of the code in multiple configuration. Use union for this. the compile-time failure is good here. it's means your code is not generic. specially when you want to keep code running on multiple soc/arch keep compiling no matter the configuration #ifdef in the code is a really bad habit 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 7.5.2011 03:50, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 12:19 Fri 06 May , Arnaud Lacombe wrote: >> Why would it be a good thing ? >> >> Most configuration-dependent code inside functions tends to be moved >> to a static inline already, which get conditionally defined based on >> the CONFIG_<foo>. If it is not, then the code is badly architectured >> (-> bad). Using that if(xxx) notation would also lead to yet more >> heavily indented function (-> bad). Moreover, this introduces >> yet-another way to check for an information (-> bad), and you will end >> up with mixing the config_is_<xxx> notation inside a function >> declaration, and CONFIG_<xxx> when not inside a function (-> bad) >> >> Actually, this is even worse than that as you'll not be able to hide >> structure (or structure members) inside CONFIG_<xxx> and use that >> structure (or structure members) in config_is_<xxx> protected block >> without causing compile-time failure. > sorry but conditionnal structure members is bad practice > you save nearly no space nut for the test of the code in multiple > configuration. Use union for this. > > the compile-time failure is good here. it's means your code is not generic. > > specially when you want to keep code running on multiple soc/arch keep compiling > no matter the configuration > > #ifdef in the code is a really bad habit Do you have proof of concept patches that make use of the config_is_xxx macros? Acked by the respective subsystem maintainers? It would be a good idea to send them along to show that this feature is going to be actually used. 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
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> writes: > this will allow to use to use > > if(config_is_xxx()) > if(config_is_xxx_module()) > > in the code instead of > > #ifdef CONFIG_xxx > #ifdef CONFIG_xxx_MODULE Very nice idea. Acked-by: Andi Kleen <ak@linux.intel.com> -Andi
Hi, On Fri, May 6, 2011 at 1:03 AM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > this will allow to use to use > > if(config_is_xxx()) > if(config_is_xxx_module()) > > in the code instead of > > #ifdef CONFIG_xxx > #ifdef CONFIG_xxx_MODULE > > and now let the compiler remove the non usefull code and not the > pre-processor > For the record, there is at least one report of dead code elimination regression in GCC 4.4.x: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42494 - Arnaud > as done in the mach-types for arm as exmaple > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > --- > v2: > > use config_is > > add support of config_is_xxxx_module > > Best Regards, > J. > scripts/kconfig/confdata.c | 29 +++++++++++++++++++++++++++++ > 1 files changed, 29 insertions(+), 0 deletions(-) > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index 61c35bf..11b8b31 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -778,6 +778,29 @@ out: > return res; > } > > +static void conf_write_function_autoconf(FILE *out, char* conf, char* name, > + int val) > +{ > + char c; > + char *tmp, *d; > + > + d = strdup(conf); > + tmp = d; > + while ((c = *conf++)) > + *d++ = tolower(c); > + > + fprintf(out, "#define %sis_", tmp); > + free(tmp); > + > + d = strdup(name); > + tmp = d; > + while ((c = *name++)) > + *d++ = tolower(c); > + fprintf(out, "%s%s() %d\n", tmp, (val > 1) ? "_module" : "", > + val ? 1 : 0); > + free(tmp); > +} > + > int conf_write_autoconf(void) > { > struct symbol *sym; > @@ -786,6 +809,7 @@ int conf_write_autoconf(void) > FILE *out, *tristate, *out_h; > time_t now; > int i; > + int fct_val; > > sym_clear_all_valid(); > > @@ -829,6 +853,7 @@ int conf_write_autoconf(void) > rootmenu.prompt->text, ctime(&now)); > > for_all_symbols(i, sym) { > + fct_val = 1; > sym_calc_value(sym); > if (!(sym->flags & SYMBOL_WRITE) || !sym->name) > continue; > @@ -842,12 +867,14 @@ 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", > CONFIG_, sym->name); > fprintf(out_h, "#define %s%s_MODULE 1\n", > CONFIG_, sym->name); > + fct_val = 2; > break; > case yes: > if (sym->type == S_TRISTATE) > @@ -874,8 +901,10 @@ 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); > -- > 1.7.4.1 > > -- > 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 > -- 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 Fri, May 06, 2011 at 07:03:49AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > this will allow to use to use > > if(config_is_xxx()) > if(config_is_xxx_module()) > > in the code instead of > > #ifdef CONFIG_xxx > #ifdef CONFIG_xxx_MODULE > > and now let the compiler remove the non usefull code and not the > pre-processor > > as done in the mach-types for arm as exmaple > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> I pushed this to kbuild-2.6.git#kconfig, but I found an issue that needs to be fixed before we start using this feature: scripts/basic/fixdep scans source files for occurences of CONFIG_xxx and adds dependencies on include/config/xxx. It needs to be taught to match config_is_xxx as well. 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 16:45 Tue 17 May , Michal Marek wrote: > On Fri, May 06, 2011 at 07:03:49AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > this will allow to use to use > > > > if(config_is_xxx()) > > if(config_is_xxx_module()) > > > > in the code instead of > > > > #ifdef CONFIG_xxx > > #ifdef CONFIG_xxx_MODULE > > > > and now let the compiler remove the non usefull code and not the > > pre-processor > > > > as done in the mach-types for arm as exmaple > > > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > > I pushed this to kbuild-2.6.git#kconfig, but I found an issue that needs > to be fixed before we start using this feature: scripts/basic/fixdep > scans source files for occurences of CONFIG_xxx and adds dependencies on > include/config/xxx. It needs to be taught to match config_is_xxx as > well. ok I take a look as I'm some patch that use it 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
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 61c35bf..11b8b31 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -778,6 +778,29 @@ out: return res; } +static void conf_write_function_autoconf(FILE *out, char* conf, char* name, + int val) +{ + char c; + char *tmp, *d; + + d = strdup(conf); + tmp = d; + while ((c = *conf++)) + *d++ = tolower(c); + + fprintf(out, "#define %sis_", tmp); + free(tmp); + + d = strdup(name); + tmp = d; + while ((c = *name++)) + *d++ = tolower(c); + fprintf(out, "%s%s() %d\n", tmp, (val > 1) ? "_module" : "", + val ? 1 : 0); + free(tmp); +} + int conf_write_autoconf(void) { struct symbol *sym; @@ -786,6 +809,7 @@ int conf_write_autoconf(void) FILE *out, *tristate, *out_h; time_t now; int i; + int fct_val; sym_clear_all_valid(); @@ -829,6 +853,7 @@ int conf_write_autoconf(void) rootmenu.prompt->text, ctime(&now)); for_all_symbols(i, sym) { + fct_val = 1; sym_calc_value(sym); if (!(sym->flags & SYMBOL_WRITE) || !sym->name) continue; @@ -842,12 +867,14 @@ 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", CONFIG_, sym->name); fprintf(out_h, "#define %s%s_MODULE 1\n", CONFIG_, sym->name); + fct_val = 2; break; case yes: if (sym->type == S_TRISTATE) @@ -874,8 +901,10 @@ 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);
this will allow to use to use if(config_is_xxx()) if(config_is_xxx_module()) in the code instead of #ifdef CONFIG_xxx #ifdef CONFIG_xxx_MODULE and now let the compiler remove the non usefull code and not the pre-processor as done in the mach-types for arm as exmaple Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> --- v2: use config_is add support of config_is_xxxx_module Best Regards, J. scripts/kconfig/confdata.c | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)