diff mbox series

stdio-common/printf-prs.c: Simplify test

Message ID 20210324180718.28006-1-alx.manpages@gmail.com
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers
Series stdio-common/printf-prs.c: Simplify test |

Commit Message

Alejandro Colomar March 24, 2021, 6:07 p.m. UTC 
  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(-)

Comments

Adhemerval Zanella Netto March 29, 2021, 7:32 p.m. UTC | #1 
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);
>
Alejandro Colomar March 29, 2021, 8:35 p.m. UTC | #2 
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
Adhemerval Zanella Netto March 29, 2021, 8:43 p.m. UTC | #3 
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 mbox series

Patch

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);