| Message ID | 1517877294-4826-2-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Kconfig unit tests | expand |
On Tue, Feb 6, 2018 at 1:34 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > These messages should be directed to stderr. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > scripts/kconfig/conf.c | 18 +++++++++++------- > scripts/kconfig/zconf.l | 27 +++++++++++++++------------ > 2 files changed, 26 insertions(+), 19 deletions(-) > > diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c > index 307bc3f..90a76aa2 100644 > --- a/scripts/kconfig/conf.c > +++ b/scripts/kconfig/conf.c > @@ -75,9 +75,11 @@ static void strip(char *str) > static void check_stdin(void) > { > if (!valid_stdin) { > - printf(_("aborted!\n\n")); > - printf(_("Console input/output is redirected. ")); > - printf(_("Run 'make oldconfig' to update configuration.\n\n")); > + fprintf(stderr, > + _("Aborted!\n" > + "Console input/output is redirected.\n" > + "Run 'make oldconfig' to update configuration.\n\n") > + ); This could use fputs() too, moving the stderr to the last argument. I think the _() thingies around the strings are for gettext (https://www.gnu.org/software/gettext/manual/html_node/Mark-Keywords.html). This would break it if there are existing translations, since the msgids change. More practically, I doubt anyone is translating these tools. IMO we should remove the gettext stuff unless we find traces of translations. > exit(1); > } > } > @@ -565,7 +567,7 @@ int main(int ac, char **av) > } > } > if (ac == optind) { > - printf(_("%s: Kconfig file missing\n"), av[0]); > + fprintf(stderr, _("%s: Kconfig file missing\n"), av[0]); > conf_usage(progname); > exit(1); > } > @@ -590,9 +592,11 @@ int main(int ac, char **av) > if (!defconfig_file) > defconfig_file = conf_get_default_confname(); > if (conf_read(defconfig_file)) { > - printf(_("***\n" > - "*** Can't find default configuration \"%s\"!\n" > - "***\n"), defconfig_file); > + fprintf(stderr, > + _("***\n" > + "*** Can't find default configuration \"%s\"!\n" > + "***\n"), > + defconfig_file); > exit(1); > } > break; > diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l > index 07e074d..0ba4900 100644 > --- a/scripts/kconfig/zconf.l > +++ b/scripts/kconfig/zconf.l > @@ -184,7 +184,9 @@ n [A-Za-z0-9_-] > append_string(yytext, 1); > } > \n { > - printf("%s:%d:warning: multi-line strings not supported\n", zconf_curname(), zconf_lineno()); > + fprintf(stderr, > + "%s:%d:warning: multi-line strings not supported\n", > + zconf_curname(), zconf_lineno()); Whether stuff is translated seems inconsistent too. > current_file->lineno++; > BEGIN(INITIAL); > return T_EOL; > @@ -294,7 +296,7 @@ void zconf_initscan(const char *name) > { > yyin = zconf_fopen(name); > if (!yyin) { > - printf("can't find file %s\n", name); > + fprintf(stderr, "can't find file %s\n", name); > exit(1); > } > > @@ -315,8 +317,8 @@ void zconf_nextfile(const char *name) > current_buf->state = YY_CURRENT_BUFFER; > yyin = zconf_fopen(file->name); > if (!yyin) { > - printf("%s:%d: can't open file \"%s\"\n", > - zconf_curname(), zconf_lineno(), file->name); > + fprintf(stderr, "%s:%d: can't open file \"%s\"\n", > + zconf_curname(), zconf_lineno(), file->name); > exit(1); > } > yy_switch_to_buffer(yy_create_buffer(yyin, YY_BUF_SIZE)); > @@ -325,20 +327,21 @@ void zconf_nextfile(const char *name) > > for (iter = current_file->parent; iter; iter = iter->parent ) { > if (!strcmp(current_file->name,iter->name) ) { > - printf("%s:%d: recursive inclusion detected. " > - "Inclusion path:\n current file : '%s'\n", > - zconf_curname(), zconf_lineno(), > - zconf_curname()); > + fprintf(stderr, > + "%s:%d: recursive inclusion detected. " > + "Inclusion path:\n current file : '%s'\n", > + zconf_curname(), zconf_lineno(), > + zconf_curname()); > iter = current_file->parent; > while (iter && \ > strcmp(iter->name,current_file->name)) { > - printf(" included from: '%s:%d'\n", > - iter->name, iter->lineno-1); > + fprintf(stderr, " included from: '%s:%d'\n", > + iter->name, iter->lineno-1); > iter = iter->parent; > } > if (iter) > - printf(" included from: '%s:%d'\n", > - iter->name, iter->lineno+1); > + fprintf(stderr, " included from: '%s:%d'\n", > + iter->name, iter->lineno+1); > exit(1); > } > } > -- > 2.7.4 > The unrelated gettext stuff aside: Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> Cheers, Ulf
2018-02-08 5:24 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Tue, Feb 6, 2018 at 1:34 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> These messages should be directed to stderr. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> scripts/kconfig/conf.c | 18 +++++++++++------- >> scripts/kconfig/zconf.l | 27 +++++++++++++++------------ >> 2 files changed, 26 insertions(+), 19 deletions(-) >> >> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c >> index 307bc3f..90a76aa2 100644 >> --- a/scripts/kconfig/conf.c >> +++ b/scripts/kconfig/conf.c >> @@ -75,9 +75,11 @@ static void strip(char *str) >> static void check_stdin(void) >> { >> if (!valid_stdin) { >> - printf(_("aborted!\n\n")); >> - printf(_("Console input/output is redirected. ")); >> - printf(_("Run 'make oldconfig' to update configuration.\n\n")); >> + fprintf(stderr, >> + _("Aborted!\n" >> + "Console input/output is redirected.\n" >> + "Run 'make oldconfig' to update configuration.\n\n") >> + ); > > This could use fputs() too, moving the stderr to the last argument. In general, I am not keen on replacement of (f)printf with (f)puts. If '%' does not appear in the format literal, compiler will automatically replace the function call with a faster one. My compiler replaced both fprintf(stderr, "blah\n") and fputs("blah\n", stderr) with fwrite. At this moment, right, the compiler cannot optimize it because it never knows the result of _(...). If we rip off the gettext things, it will be optimized. > I think the _() thingies around the strings are for gettext > (https://www.gnu.org/software/gettext/manual/html_node/Mark-Keywords.html). > This would break it if there are existing translations, since the > msgids change. Right. I will keep inside _(...) as-is just in case to not break gettext. > More practically, I doubt anyone is translating these tools. IMO we > should remove the gettext stuff unless we find traces of translations. I agree. Probably, it should not hurt to eliminate gettext, but out of scope of this series. >> exit(1); >> } >> } >> @@ -565,7 +567,7 @@ int main(int ac, char **av) >> } >> } >> if (ac == optind) { >> - printf(_("%s: Kconfig file missing\n"), av[0]); >> + fprintf(stderr, _("%s: Kconfig file missing\n"), av[0]); >> conf_usage(progname); >> exit(1); >> } >> @@ -590,9 +592,11 @@ int main(int ac, char **av) >> if (!defconfig_file) >> defconfig_file = conf_get_default_confname(); >> if (conf_read(defconfig_file)) { >> - printf(_("***\n" >> - "*** Can't find default configuration \"%s\"!\n" >> - "***\n"), defconfig_file); >> + fprintf(stderr, >> + _("***\n" >> + "*** Can't find default configuration \"%s\"!\n" >> + "***\n"), >> + defconfig_file); >> exit(1); >> } >> break; >> diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l >> index 07e074d..0ba4900 100644 >> --- a/scripts/kconfig/zconf.l >> +++ b/scripts/kconfig/zconf.l >> @@ -184,7 +184,9 @@ n [A-Za-z0-9_-] >> append_string(yytext, 1); >> } >> \n { >> - printf("%s:%d:warning: multi-line strings not supported\n", zconf_curname(), zconf_lineno()); >> + fprintf(stderr, >> + "%s:%d:warning: multi-line strings not supported\n", >> + zconf_curname(), zconf_lineno()); > > Whether stuff is translated seems inconsistent too. Right. Many people change the same tool, it is difficult to keep the consistency. >> current_file->lineno++; >> BEGIN(INITIAL); >> return T_EOL; >> @@ -294,7 +296,7 @@ void zconf_initscan(const char *name) >> { >> yyin = zconf_fopen(name); >> if (!yyin) { >> - printf("can't find file %s\n", name); >> + fprintf(stderr, "can't find file %s\n", name); >> exit(1); >> } >> >> @@ -315,8 +317,8 @@ void zconf_nextfile(const char *name) >> current_buf->state = YY_CURRENT_BUFFER; >> yyin = zconf_fopen(file->name); >> if (!yyin) { >> - printf("%s:%d: can't open file \"%s\"\n", >> - zconf_curname(), zconf_lineno(), file->name); >> + fprintf(stderr, "%s:%d: can't open file \"%s\"\n", >> + zconf_curname(), zconf_lineno(), file->name); >> exit(1); >> } >> yy_switch_to_buffer(yy_create_buffer(yyin, YY_BUF_SIZE)); >> @@ -325,20 +327,21 @@ void zconf_nextfile(const char *name) >> >> for (iter = current_file->parent; iter; iter = iter->parent ) { >> if (!strcmp(current_file->name,iter->name) ) { >> - printf("%s:%d: recursive inclusion detected. " >> - "Inclusion path:\n current file : '%s'\n", >> - zconf_curname(), zconf_lineno(), >> - zconf_curname()); >> + fprintf(stderr, >> + "%s:%d: recursive inclusion detected. " >> + "Inclusion path:\n current file : '%s'\n", >> + zconf_curname(), zconf_lineno(), >> + zconf_curname()); >> iter = current_file->parent; >> while (iter && \ >> strcmp(iter->name,current_file->name)) { >> - printf(" included from: '%s:%d'\n", >> - iter->name, iter->lineno-1); >> + fprintf(stderr, " included from: '%s:%d'\n", >> + iter->name, iter->lineno-1); >> iter = iter->parent; >> } >> if (iter) >> - printf(" included from: '%s:%d'\n", >> - iter->name, iter->lineno+1); >> + fprintf(stderr, " included from: '%s:%d'\n", >> + iter->name, iter->lineno+1); >> exit(1); >> } >> } >> -- >> 2.7.4 >> > > The unrelated gettext stuff aside: > > Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> > > Cheers, > Ulf > -- > 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 -- Best Regards Masahiro Yamada
On Thu, Feb 8, 2018 at 2:49 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-08 5:24 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Tue, Feb 6, 2018 at 1:34 AM, Masahiro Yamada >> <yamada.masahiro@socionext.com> wrote: >>> These messages should be directed to stderr. >>> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>> --- >>> >>> scripts/kconfig/conf.c | 18 +++++++++++------- >>> scripts/kconfig/zconf.l | 27 +++++++++++++++------------ >>> 2 files changed, 26 insertions(+), 19 deletions(-) >>> >>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c >>> index 307bc3f..90a76aa2 100644 >>> --- a/scripts/kconfig/conf.c >>> +++ b/scripts/kconfig/conf.c >>> @@ -75,9 +75,11 @@ static void strip(char *str) >>> static void check_stdin(void) >>> { >>> if (!valid_stdin) { >>> - printf(_("aborted!\n\n")); >>> - printf(_("Console input/output is redirected. ")); >>> - printf(_("Run 'make oldconfig' to update configuration.\n\n")); >>> + fprintf(stderr, >>> + _("Aborted!\n" >>> + "Console input/output is redirected.\n" >>> + "Run 'make oldconfig' to update configuration.\n\n") >>> + ); >> >> This could use fputs() too, moving the stderr to the last argument. > > > In general, I am not keen on replacement of (f)printf with (f)puts. > > If '%' does not appear in the format literal, compiler will automatically > replace the function call with a faster one. > > My compiler replaced both fprintf(stderr, "blah\n") and fputs("blah\n", stderr) > with fwrite. > > At this moment, right, the compiler cannot optimize it > because it never knows the result of _(...). > > If we rip off the gettext things, it will be optimized. No problem with me. Just some personal OCD. :) > > > >> I think the _() thingies around the strings are for gettext >> (https://www.gnu.org/software/gettext/manual/html_node/Mark-Keywords.html). >> This would break it if there are existing translations, since the >> msgids change. > > Right. I will keep inside _(...) as-is just in case > to not break gettext. > > > > >> More practically, I doubt anyone is translating these tools. IMO we >> should remove the gettext stuff unless we find traces of translations. > > I agree. Probably, it should not hurt to eliminate gettext, > but out of scope of this series. Yeah, out of scope. > > >>> exit(1); >>> } >>> } >>> @@ -565,7 +567,7 @@ int main(int ac, char **av) >>> } >>> } >>> if (ac == optind) { >>> - printf(_("%s: Kconfig file missing\n"), av[0]); >>> + fprintf(stderr, _("%s: Kconfig file missing\n"), av[0]); >>> conf_usage(progname); >>> exit(1); >>> } >>> @@ -590,9 +592,11 @@ int main(int ac, char **av) >>> if (!defconfig_file) >>> defconfig_file = conf_get_default_confname(); >>> if (conf_read(defconfig_file)) { >>> - printf(_("***\n" >>> - "*** Can't find default configuration \"%s\"!\n" >>> - "***\n"), defconfig_file); >>> + fprintf(stderr, >>> + _("***\n" >>> + "*** Can't find default configuration \"%s\"!\n" >>> + "***\n"), >>> + defconfig_file); >>> exit(1); >>> } >>> break; >>> diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l >>> index 07e074d..0ba4900 100644 >>> --- a/scripts/kconfig/zconf.l >>> +++ b/scripts/kconfig/zconf.l >>> @@ -184,7 +184,9 @@ n [A-Za-z0-9_-] >>> append_string(yytext, 1); >>> } >>> \n { >>> - printf("%s:%d:warning: multi-line strings not supported\n", zconf_curname(), zconf_lineno()); >>> + fprintf(stderr, >>> + "%s:%d:warning: multi-line strings not supported\n", >>> + zconf_curname(), zconf_lineno()); >> >> Whether stuff is translated seems inconsistent too. > > > Right. Many people change the same tool, it is difficult to > keep the consistency. > > > > >>> current_file->lineno++; >>> BEGIN(INITIAL); >>> return T_EOL; >>> @@ -294,7 +296,7 @@ void zconf_initscan(const char *name) >>> { >>> yyin = zconf_fopen(name); >>> if (!yyin) { >>> - printf("can't find file %s\n", name); >>> + fprintf(stderr, "can't find file %s\n", name); >>> exit(1); >>> } >>> >>> @@ -315,8 +317,8 @@ void zconf_nextfile(const char *name) >>> current_buf->state = YY_CURRENT_BUFFER; >>> yyin = zconf_fopen(file->name); >>> if (!yyin) { >>> - printf("%s:%d: can't open file \"%s\"\n", >>> - zconf_curname(), zconf_lineno(), file->name); >>> + fprintf(stderr, "%s:%d: can't open file \"%s\"\n", >>> + zconf_curname(), zconf_lineno(), file->name); >>> exit(1); >>> } >>> yy_switch_to_buffer(yy_create_buffer(yyin, YY_BUF_SIZE)); >>> @@ -325,20 +327,21 @@ void zconf_nextfile(const char *name) >>> >>> for (iter = current_file->parent; iter; iter = iter->parent ) { >>> if (!strcmp(current_file->name,iter->name) ) { >>> - printf("%s:%d: recursive inclusion detected. " >>> - "Inclusion path:\n current file : '%s'\n", >>> - zconf_curname(), zconf_lineno(), >>> - zconf_curname()); >>> + fprintf(stderr, >>> + "%s:%d: recursive inclusion detected. " >>> + "Inclusion path:\n current file : '%s'\n", >>> + zconf_curname(), zconf_lineno(), >>> + zconf_curname()); >>> iter = current_file->parent; >>> while (iter && \ >>> strcmp(iter->name,current_file->name)) { >>> - printf(" included from: '%s:%d'\n", >>> - iter->name, iter->lineno-1); >>> + fprintf(stderr, " included from: '%s:%d'\n", >>> + iter->name, iter->lineno-1); >>> iter = iter->parent; >>> } >>> if (iter) >>> - printf(" included from: '%s:%d'\n", >>> - iter->name, iter->lineno+1); >>> + fprintf(stderr, " included from: '%s:%d'\n", >>> + iter->name, iter->lineno+1); >>> exit(1); >>> } >>> } >>> -- >>> 2.7.4 >>> >> >> The unrelated gettext stuff aside: >> >> Reviewed-by: Ulf Magnusson <ulfalizer@gmail.com> >> >> Cheers, >> Ulf >> -- >> 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 > > > > -- > Best Regards > Masahiro Yamada Cheers, Ulf
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 307bc3f..90a76aa2 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -75,9 +75,11 @@ static void strip(char *str) static void check_stdin(void) { if (!valid_stdin) { - printf(_("aborted!\n\n")); - printf(_("Console input/output is redirected. ")); - printf(_("Run 'make oldconfig' to update configuration.\n\n")); + fprintf(stderr, + _("Aborted!\n" + "Console input/output is redirected.\n" + "Run 'make oldconfig' to update configuration.\n\n") + ); exit(1); } } @@ -565,7 +567,7 @@ int main(int ac, char **av) } } if (ac == optind) { - printf(_("%s: Kconfig file missing\n"), av[0]); + fprintf(stderr, _("%s: Kconfig file missing\n"), av[0]); conf_usage(progname); exit(1); } @@ -590,9 +592,11 @@ int main(int ac, char **av) if (!defconfig_file) defconfig_file = conf_get_default_confname(); if (conf_read(defconfig_file)) { - printf(_("***\n" - "*** Can't find default configuration \"%s\"!\n" - "***\n"), defconfig_file); + fprintf(stderr, + _("***\n" + "*** Can't find default configuration \"%s\"!\n" + "***\n"), + defconfig_file); exit(1); } break; diff --git a/scripts/kconfig/zconf.l b/scripts/kconfig/zconf.l index 07e074d..0ba4900 100644 --- a/scripts/kconfig/zconf.l +++ b/scripts/kconfig/zconf.l @@ -184,7 +184,9 @@ n [A-Za-z0-9_-] append_string(yytext, 1); } \n { - printf("%s:%d:warning: multi-line strings not supported\n", zconf_curname(), zconf_lineno()); + fprintf(stderr, + "%s:%d:warning: multi-line strings not supported\n", + zconf_curname(), zconf_lineno()); current_file->lineno++; BEGIN(INITIAL); return T_EOL; @@ -294,7 +296,7 @@ void zconf_initscan(const char *name) { yyin = zconf_fopen(name); if (!yyin) { - printf("can't find file %s\n", name); + fprintf(stderr, "can't find file %s\n", name); exit(1); } @@ -315,8 +317,8 @@ void zconf_nextfile(const char *name) current_buf->state = YY_CURRENT_BUFFER; yyin = zconf_fopen(file->name); if (!yyin) { - printf("%s:%d: can't open file \"%s\"\n", - zconf_curname(), zconf_lineno(), file->name); + fprintf(stderr, "%s:%d: can't open file \"%s\"\n", + zconf_curname(), zconf_lineno(), file->name); exit(1); } yy_switch_to_buffer(yy_create_buffer(yyin, YY_BUF_SIZE)); @@ -325,20 +327,21 @@ void zconf_nextfile(const char *name) for (iter = current_file->parent; iter; iter = iter->parent ) { if (!strcmp(current_file->name,iter->name) ) { - printf("%s:%d: recursive inclusion detected. " - "Inclusion path:\n current file : '%s'\n", - zconf_curname(), zconf_lineno(), - zconf_curname()); + fprintf(stderr, + "%s:%d: recursive inclusion detected. " + "Inclusion path:\n current file : '%s'\n", + zconf_curname(), zconf_lineno(), + zconf_curname()); iter = current_file->parent; while (iter && \ strcmp(iter->name,current_file->name)) { - printf(" included from: '%s:%d'\n", - iter->name, iter->lineno-1); + fprintf(stderr, " included from: '%s:%d'\n", + iter->name, iter->lineno-1); iter = iter->parent; } if (iter) - printf(" included from: '%s:%d'\n", - iter->name, iter->lineno+1); + fprintf(stderr, " included from: '%s:%d'\n", + iter->name, iter->lineno+1); exit(1); } }
These messages should be directed to stderr. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- scripts/kconfig/conf.c | 18 +++++++++++------- scripts/kconfig/zconf.l | 27 +++++++++++++++------------ 2 files changed, 26 insertions(+), 19 deletions(-) -- 2.7.4