[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Mon, 03 Oct 2005 16:44:08 +0200
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.
That's easy to fix.
is_negative = (*p == '-');
is_negative = (*p == '-' && ISDIGIT (p));
> 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.
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.
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)
char buf[INT_BUFSIZE_BOUND (uintmax_t)];
+ bool is_negative;
+ char const *p = s;
+ char const *s_diag;
+ while (ISSPACE (*p))
+ /* Save a copy solely for diagnostics. */
+ s_diag = p;
+ is_negative = (*p == '-' && ISDIGIT (p));
+ 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));
- error (0, 0, _("%s is not a valid positive integer"), quote (s));
+ error (0, 0, _("invalid argument: %s"), quote (s_diag));
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));
- Re: factor [Was: coreutils v5.2.1 - stat.c], (continued)
- Re: factor, Jim Meyering, 2005/10/03
- Re: factor, ThMO, 2005/10/03