Re: factor

[Jim Meyering]

ThMO <address@hidden> wrote:
> Hello Jim and others listening,
>
>> [...]
>> I'd be more interested in a patch to make GNU factor accept negative
>> numbers if it weren't so invasive.  How about just examining the string
>> for a leading `-' (possibly after white space), remembering that, and
>> skipping over it.  Then the existing unsigned code will work just fine.
>> Patch below.
>
> That's a possibility too.
>
> But your patch introduces a bug - it's possible now to give an invalid
> number on the command line, which would normally be rejected by strtol():
> -  ./gfactor -- '  -  130'
> -  ./gfactor -- '  -  +131'
> xstrtoumax() now skips the white spaces between `-' and the number itself,
> which is wrong in this case; so if a minus sign has been scanned, you need
> to ensure, that there is a digit following immediately.  The same applies
> for a possible `+' sign.

Good catch.
That's easy to fix.
Change this:
  is_negative = (*p == '-');
to this:
  is_negative = (*p == '-' && ISDIGIT (p[1]));

> That's the reason, why I've preferred to let this dirty work handed over
> to the conversion routines, as otherwise I'll have to reinvent the wheel
> over and over again...

But your patch limited the maximum magnitude to that of intmax_t,
which is half that of the current limit.

> Another ugly looking thing, although it's not quite an error:
>   ./gfactor -- '-69ways'
> would state, that ``-69ways'' isn't a valid *positive* value, which isn't
> quite the correct error message, as
>   ./gfactor -- -69
> will work as expected, so the word `positive' should be eliminated, IMHO.

Yes, indeed.
Here's a new and improved patch :-)
[the changed message makes at least one test fail,
 but I'll fix that when I add new tests. ]

Thanks for the quick review.

Index: src/factor.c
===================================================================
RCS file: /fetish/cu/src/factor.c,v
retrieving revision 1.77
diff -u -p -r1.77 factor.c
--- src/factor.c        3 Oct 2005 12:12:14 -0000       1.77
+++ src/factor.c        3 Oct 2005 14:25:21 -0000
@@ -149,17 +149,32 @@ print_factors (const char *s)
   size_t i;
   char buf[INT_BUFSIZE_BOUND (uintmax_t)];
   strtol_error err;
+  bool is_negative;
+  char const *p = s;
+  char const *s_diag;
+
+  while (ISSPACE (*p))
+    ++p;
+
+  /* Save a copy solely for diagnostics.  */
+  s_diag = p;
+  is_negative = (*p == '-' && ISDIGIT (p[1]));
+  p += is_negative;
 
-  if ((err = xstrtoumax (s, NULL, 10, &n, "")) != LONGINT_OK)
+  if ((err = xstrtoumax (p, NULL, 10, &n, "")) != LONGINT_OK
+      || n == 0)
     {
       if (err == LONGINT_OVERFLOW)
-       error (0, 0, _("%s is too large"), quote (s));
+       error (0, 0, _("%s is too large"), quote (s_diag));
       else
-       error (0, 0, _("%s is not a valid positive integer"), quote (s));
+       error (0, 0, _("invalid argument: %s"), quote (s_diag));
       return false;
     }
   n_factors = factor (n, MAX_N_FACTORS, factors);
-  printf ("%s:", umaxtostr (n, buf));
+  printf ("%s%s:%s",
+         (is_negative ? "-" : ""),
+         umaxtostr (n, buf),
+         (is_negative ? " -1" : ""));
   for (i = 0; i < n_factors; i++)
     printf (" %s", umaxtostr (factors[i], buf));
   putchar ('\n');