On Tue, Mar 27, 2018 at 10:48:51PM +0200, Tobias Stoeckmann wrote:
> On Tue, Mar 27, 2018 at 10:36:17PM +0200, Ingo Schwarze wrote:
> > > + int sig;
> >
> > Drop this variable, it does no good but only harm.
>
> Yeah, too bad that I didn't notice this left-over from a previous
> development step. Strange enough that regression tests didn't
> choke on it...
>
> > > + /* skip leading zeros */
> > > + while (*p == '0' && isdigit((unsigned char)p[1]))
> > > + p++;
> > > +
> > > + /* turn 0 always positive */
> > > + if (*p == '0' && !isdigit((unsigned char)p[1]))
> > > + sig = 1;
>
> As tb@ pointed out, the !isdigit check can be removed as well.
>
> > > + c = strncmp(p1, p2, len1);
> >
> > That's another bug:
> >
> > schwarze@isnote $ /bin/test -1 -gt -2 ; echo $?
> > 1
> > schwarze@isnote $ /bin/test 1 -gt 2 ; echo $?
> > 1
>
> It's really great to have people who do extensive reviews. Thanks
> a lot for spotting this. I have added it to the regression tests.
>
> > With these changes, OK schwarze@. See below for the patch that
> > i tested.
>
> Modified with tb@'s remark. Also I changed the error message
> "bad number" to "invalid", so we stay fully in sync with strtonum's
> error messages.
>
> > Given that breaking test(1) might cause hard-to-find breakage
> > deep down in build systems and scripts, you might with a to
> > have a second OK for commit, though.
>
> Definitely, I'm not out to rush this one in.
ok tb
>
>
> Index: bin/test/test.c
> ===================================================================
> RCS file: /cvs/src/bin/test/test.c,v
> retrieving revision 1.18
> diff -u -p -u -p -r1.18 test.c
> --- bin/test/test.c 24 Jul 2017 22:15:52 -0000 1.18
> +++ bin/test/test.c 27 Mar 2018 20:43:38 -0000
> @@ -16,6 +16,7 @@
> #include <unistd.h>
> #include <ctype.h>
> #include <errno.h>
> +#include <limits.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -145,6 +146,8 @@ static int aexpr(enum token n);
> static int nexpr(enum token n);
> static int binop(void);
> static int primary(enum token n);
> +static const char *getnstr(const char *, int *, size_t *);
> +static int intcmp(const char *, const char *);
> static int filstat(char *nm, enum token mode);
> static int getn(const char *s);
> static int newerf(const char *, const char *);
> @@ -290,6 +293,70 @@ primary(enum token n)
> return strlen(*t_wp) > 0;
> }
>
> +static const char *
> +getnstr(const char *s, int *signum, size_t *len)
> +{
> + const char *p, *start;
> +
> + /* skip leading whitespaces */
> + p = s;
> + while (isspace((unsigned char)*p))
> + p++;
> +
> + /* accept optional sign */
> + if (*p == '-') {
> + *signum = -1;
> + p++;
> + } else {
> + *signum = 1;
> + if (*p == '+')
> + p++;
> + }
> +
> + /* skip leading zeros */
> + while (*p == '0' && isdigit((unsigned char)p[1]))
> + p++;
> +
> + /* turn 0 always positive */
> + if (*p == '0')
> + *signum = 1;
> +
> + start = p;
> + while (isdigit((unsigned char)*p))
> + p++;
> + *len = p - start;
> +
> + /* allow trailing whitespaces */
> + while (isspace((unsigned char)*p))
> + p++;
> +
> + /* validate number */
> + if (*p != '\0' || *start == '\0')
> + errx(2, "%s: invalid", s);
> +
> + return start;
> +}
> +
> +static int
> +intcmp(const char *opnd1, const char *opnd2)
> +{
> + const char *p1, *p2;
> + size_t len1, len2;
> + int c, sig1, sig2;
> +
> + p1 = getnstr(opnd1, &sig1, &len1);
> + p2 = getnstr(opnd2, &sig2, &len2);
> +
> + if (sig1 != sig2)
> + c = sig1;
> + else if (len1 != len2)
> + c = (len1 < len2) ? -sig1 : sig1;
> + else
> + c = strncmp(p1, p2, len1) * sig1;
> +
> + return c;
> +}
> +
> static int
> binop(void)
> {
> @@ -313,17 +380,17 @@ binop(void)
> case STRGT:
> return strcmp(opnd1, opnd2) > 0;
> case INTEQ:
> - return getn(opnd1) == getn(opnd2);
> + return intcmp(opnd1, opnd2) == 0;
> case INTNE:
> - return getn(opnd1) != getn(opnd2);
> + return intcmp(opnd1, opnd2) != 0;
> case INTGE:
> - return getn(opnd1) >= getn(opnd2);
> + return intcmp(opnd1, opnd2) >= 0;
> case INTGT:
> - return getn(opnd1) > getn(opnd2);
> + return intcmp(opnd1, opnd2) > 0;
> case INTLE:
> - return getn(opnd1) <= getn(opnd2);
> + return intcmp(opnd1, opnd2) <= 0;
> case INTLT:
> - return getn(opnd1) < getn(opnd2);
> + return intcmp(opnd1, opnd2) < 0;
> case FILNT:
> return newerf(opnd1, opnd2);
> case FILOT:
> @@ -455,22 +522,26 @@ t_lex(char *s)
> static int
> getn(const char *s)
> {
> - char *p;
> - long r;
> -
> - errno = 0;
> - r = strtol(s, &p, 10);
> -
> - if (errno != 0)
> - errx(2, "%s: out of range", s);
> -
> - while (isspace((unsigned char)*p))
> - p++;
> + char buf[32];
> + const char *errstr, *p;
> + size_t len;
> + int r, sig;
> +
> + p = getnstr(s, &sig, &len);
> + if (sig != 1)
> + errstr = "too small";
> + else if (len >= sizeof(buf))
> + errstr = "too large";
> + else {
> + strlcpy(buf, p, sizeof(buf));
> + buf[len] = '\0';
> + r = strtonum(buf, 0, INT_MAX, &errstr);
> + }
>
> - if (*p)
> - errx(2, "%s: bad number", s);
> + if (errstr != NULL)
> + errx(2, "%s: %s", s, errstr);
>
> - return (int) r;
> + return r;
> }
>
> static int
> Index: regress/bin/test/TEST.sh
> ===================================================================
> RCS file: /cvs/src/regress/bin/test/TEST.sh,v
> retrieving revision 1.2
> diff -u -p -u -p -r1.2 TEST.sh
> --- regress/bin/test/TEST.sh 25 Nov 2014 23:09:22 -0000 1.2
> +++ regress/bin/test/TEST.sh 27 Mar 2018 20:43:38 -0000
> @@ -124,6 +124,17 @@ t 0 '0 -eq 0'
> t 1 '-5 -eq 5'
> t 0 '\( 0 -eq 0 \)'
> t 1 '1 -eq 0 -o a = a -a 1 -eq 0 -o a = aa'
> +t 0 '" +123 " -eq 123'
> +t 1 '"-123 " -gt " -1"'
> +t 0 '123 -gt -123'
> +t 0 '-0 -eq +0'
> +t 1 '+0 -gt 0'
> +t 0 '0 -eq 0'
> +t 0 '0000 -eq -0'
> +t 0 '-1 -gt -2'
> +t 1 '1 -gt 2'
> +t 1 '4294967296 -eq 0'
> +t 0 '12345678901234567890 -eq +0012345678901234567890'
>
> t 1 '"" -o ""'
> t 1 '"" -a ""'
>
