| Message ID | 20210324180718.28006-1-alx.manpages@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | stdio-common/printf-prs.c: Simplify test | expand |
On 24/03/2021 15:07, Alejandro Colomar via Libc-alpha wrote: > The test was being repeated for all the remaining code in the for loop. > Test once, and 'continue' to next iteration if necessary, so that the > following code can assume that the necessary condition is met. > > Minor changes: fix some mixed tabs & spaces to use spaces only. > > Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com> We do not use SCO, but rather Copyright assignment. And I think we will need to add a testcase for this change, this simple program below shows a complete different result with this patch applied: --- x86_64-linux-gnu$ cat test.c #include <stdio.h> #include <printf.h> #include <sys/param.h> static void print_argtype (int argtype) { if ((argtype & ~PA_FLAG_MASK) == PA_INT) printf ("PA_INT"); else if ((argtype & ~PA_FLAG_MASK) == PA_CHAR) printf ("PA_CHAR"); else if ((argtype & ~PA_FLAG_MASK) == PA_STRING) printf ("PA_STRING"); else if ((argtype & ~PA_FLAG_MASK) == PA_POINTER) printf ("PA_POINTER"); else if ((argtype & ~PA_FLAG_MASK) == PA_FLOAT) printf ("PA_FLOAT"); else if ((argtype & ~PA_FLAG_MASK) == PA_DOUBLE) printf ("PA_DOUBLE"); else printf ("UNKNOWN"); if ((argtype & PA_FLAG_MASK) == PA_FLAG_PTR) printf ("| PA_FLAG_PTR"); if ((argtype & PA_FLAG_MASK) == PA_FLAG_SHORT) printf ("| PA_FLAG_SHORT"); if ((argtype & PA_FLAG_MASK) == PA_FLAG_LONG) printf (" | PA_FLAG_LONG"); if ((argtype & PA_FLAG_MASK) == PA_FLAG_LONG_LONG) printf (" | PA_FLAG_LONG_LONG"); if ((argtype & PA_FLAG_MASK) == PA_FLAG_LONG_DOUBLE) printf (" | PA_FLAG_LONG_DOUBLE"); printf ("\n"); } static void test_parse_printf_format (int maxsize) { int argtypes[maxsize]; size_t argtypes_s = sizeof (argtypes) / sizeof (argtypes[0]); size_t s = parse_printf_format ( "%10u foo %% %+2.3f %ld %s %lld %jd %zu %ld", argtypes_s, argtypes); for (int i = 0; i < MIN (s, argtypes_s); i++) print_argtype (argtypes[i]); printf ("\n"); } int main (int argc, char *argv[]) { test_parse_printf_format (4); test_parse_printf_format (20); return 0; } x86_64-linux-gnu$ gcc -Wall test.c -o test x86_64-linux-gnu$ ./test PA_INT PA_DOUBLE PA_INT | PA_FLAG_LONG PA_STRING PA_INT PA_DOUBLE PA_INT | PA_FLAG_LONG PA_STRING PA_INT | PA_FLAG_LONG PA_INT | PA_FLAG_LONG PA_INT | PA_FLAG_LONG PA_INT | PA_FLAG_LONG x86_64-linux-gnu$ ./testrun.sh ./test PA_INT PA_INT PA_INT PA_INT UNKNOWN UNKNOWN UNKNOWN UNKNOWN UNKNOWN UNKNOWN UNKNOWN UNKNOWN > --- > stdio-common/printf-prs.c | 44 ++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/stdio-common/printf-prs.c b/stdio-common/printf-prs.c > index 1d4e00553d..a0c5c5487b 100644 > --- a/stdio-common/printf-prs.c > +++ b/stdio-common/printf-prs.c > @@ -72,30 +72,32 @@ parse_printf_format (const char *fmt, size_t n, int *argtypes) > /* Parse this spec. */ > nargs += __parse_one_specmb (f, nargs, &spec, &max_ref_arg); > > + if ((size_t) spec.width_arg >= n) > + continue; > + > /* If the width is determined by an argument this is an int. */ > - if (spec.width_arg != -1 && (size_t) spec.width_arg < n) > - argtypes[spec.width_arg] = PA_INT; > + if (spec.width_arg != -1) > + argtypes[spec.width_arg] = PA_INT; > > /* If the precision is determined by an argument this is an int. */ > - if (spec.prec_arg != -1 && (size_t) spec.prec_arg < n) > - argtypes[spec.prec_arg] = PA_INT; > - > - if ((size_t) spec.data_arg < n) > - switch (spec.ndata_args) > - { > - case 0: /* No arguments. */ > - break; > - case 1: /* One argument; we already have the type. */ > - argtypes[spec.data_arg] = spec.data_arg_type; > - break; > - default: > - /* We have more than one argument for this format spec. We must > - call the arginfo function again to determine all the types. */ > - (void) (*__printf_arginfo_table[spec.info.spec]) > - (&spec.info, n - spec.data_arg, &argtypes[spec.data_arg], > - &spec.size); > - break; > - } > + if (spec.prec_arg != -1) > + argtypes[spec.prec_arg] = PA_INT; > + > + switch (spec.ndata_args) > + { > + case 0: /* No arguments. */ > + break; > + case 1: /* One argument; we already have the type. */ > + argtypes[spec.data_arg] = spec.data_arg_type; > + break; > + default: > + /* We have more than one argument for this format spec. We must > + call the arginfo function again to determine all the types. */ > + (void) (*__printf_arginfo_table[spec.info.spec]) > + (&spec.info, n - spec.data_arg, &argtypes[spec.data_arg], > + &spec.size); > + break; > + } > } > > return MAX (nargs, max_ref_arg); >
On 3/29/21 9:32 PM, Adhemerval Zanella wrote: > > > On 24/03/2021 15:07, Alejandro Colomar via Libc-alpha wrote: >> The test was being repeated for all the remaining code in the for loop. >> Test once, and 'continue' to next iteration if necessary, so that the >> following code can assume that the necessary condition is met. >> >> Minor changes: fix some mixed tabs & spaces to use spaces only. >> >> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com> > > We do not use SCO, but rather Copyright assignment. And I think we will > need to add a testcase for this change, this simple program below shows > a complete different result with this patch applied: > Hello Adhemerval, Yes, I was completely wrong with that patch. I submitted a patch v2 after that, trying to improve the comments (without the code changes, which were wrong) to avoid confusion: <https://sourceware.org/pipermail/libc-alpha/2021-March/124359.html>. Thanks, Alex
On 29/03/2021 17:35, Alejandro Colomar (man-pages) wrote: > > > On 3/29/21 9:32 PM, Adhemerval Zanella wrote: >> >> >> On 24/03/2021 15:07, Alejandro Colomar via Libc-alpha wrote: >>> The test was being repeated for all the remaining code in the for loop. >>> Test once, and 'continue' to next iteration if necessary, so that the >>> following code can assume that the necessary condition is met. >>> >>> Minor changes: fix some mixed tabs & spaces to use spaces only. >>> >>> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com> >> >> We do not use SCO, but rather Copyright assignment. And I think we will >> need to add a testcase for this change, this simple program below shows >> a complete different result with this patch applied: >> > > Hello Adhemerval, > > Yes, I was completely wrong with that patch. I submitted a patch v2 > after that, trying to improve the comments (without the code changes, > which were wrong) to avoid confusion: > <https://sourceware.org/pipermail/libc-alpha/2021-March/124359.html>. Ok, I will review this one and mark this as superseded.
diff --git a/stdio-common/printf-prs.c b/stdio-common/printf-prs.c index 1d4e00553d..a0c5c5487b 100644 --- a/stdio-common/printf-prs.c +++ b/stdio-common/printf-prs.c @@ -72,30 +72,32 @@ parse_printf_format (const char *fmt, size_t n, int *argtypes) /* Parse this spec. */ nargs += __parse_one_specmb (f, nargs, &spec, &max_ref_arg); + if ((size_t) spec.width_arg >= n) + continue; + /* If the width is determined by an argument this is an int. */ - if (spec.width_arg != -1 && (size_t) spec.width_arg < n) - argtypes[spec.width_arg] = PA_INT; + if (spec.width_arg != -1) + argtypes[spec.width_arg] = PA_INT; /* If the precision is determined by an argument this is an int. */ - if (spec.prec_arg != -1 && (size_t) spec.prec_arg < n) - argtypes[spec.prec_arg] = PA_INT; - - if ((size_t) spec.data_arg < n) - switch (spec.ndata_args) - { - case 0: /* No arguments. */ - break; - case 1: /* One argument; we already have the type. */ - argtypes[spec.data_arg] = spec.data_arg_type; - break; - default: - /* We have more than one argument for this format spec. We must - call the arginfo function again to determine all the types. */ - (void) (*__printf_arginfo_table[spec.info.spec]) - (&spec.info, n - spec.data_arg, &argtypes[spec.data_arg], - &spec.size); - break; - } + if (spec.prec_arg != -1) + argtypes[spec.prec_arg] = PA_INT; + + switch (spec.ndata_args) + { + case 0: /* No arguments. */ + break; + case 1: /* One argument; we already have the type. */ + argtypes[spec.data_arg] = spec.data_arg_type; + break; + default: + /* We have more than one argument for this format spec. We must + call the arginfo function again to determine all the types. */ + (void) (*__printf_arginfo_table[spec.info.spec]) + (&spec.info, n - spec.data_arg, &argtypes[spec.data_arg], + &spec.size); + break; + } } return MAX (nargs, max_ref_arg);
The test was being repeated for all the remaining code in the for loop. Test once, and 'continue' to next iteration if necessary, so that the following code can assume that the necessary condition is met. Minor changes: fix some mixed tabs & spaces to use spaces only. Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com> --- stdio-common/printf-prs.c | 44 ++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 21 deletions(-)