X-Authentication-Warning:	acp3bf.physik.rwth-aachen.de: broeker owned process doing -bs
Date:	Thu, 1 Mar 2001 14:36:30 +0100 (MET)
From:	Hans-Bernhard Broeker <broeker AT physik DOT rwth-aachen DOT de>
X-Sender:	broeker AT acp3bf
To:	djgpp workers list <djgpp-workers AT delorie DOT com>
Subject:	Re: strtol fix correct?
In-Reply-To:	<Pine.SUN.3.91.1010301145132.6488B-100000@is>
Message-ID:	<Pine.LNX.4.10.10103011413530.8909-100000@acp3bf>
MIME-Version:	1.0
Reply-To:	djgpp-workers AT delorie DOT com
Errors-To:	nobody AT delorie DOT com
X-Mailing-List:	djgpp-workers AT delorie DOT com
X-Unsubscribes-To:	listserv AT delorie DOT com

On Thu, 1 Mar 2001, Eli Zaretskii wrote:

> On Thu, 1 Mar 2001, Hans-Bernhard Broeker wrote:

> > Or at least replace those "& 0xff" operations by what they *really* 
> > do: cast to unsigned char, i.e. I'd like to propose to change that
> > patched line and similar places to:
> > 
> > 	unsigned char c;
> > 	/* ... */
> > 	c = (unsigned char) *s++
> 
> This doesn't really work, unfortunately, at least not in all cases, due 
> to C promotion rules.  The code in point cannot declare c unsigned char
> (it's an int), and without that casting doesn't do any good.

A little misunderstanding, there. I had meant to replace the existing
declaration of 'c' by unsigned char.

> > I haven't check whether this triggers signedness warnings, but if it does,
> > then other, similar methods could and should be found that don't. 
> 
> Alas, that usually means almost complete rewrite of the code.  

I don't think so. The problem at hand arose because one execution path was
overlooked by whoever did those late-bug-fix changes, last time, and could
have been avoided by rigorously replacing every single instance of "c =
*s++;" by "c = (*s++) & 0xff;".

Changing s to unsigned char *, too, would reduce the number of places that
have to be changed to a single one: the initialization of 's'. All the
rest of strtol() could be left as is, and the 0xff stuff be removed.

> If someone cares enough, they are welcome to do that, now that we
> don't have to rush into a long-overdue bug-fix release.

Well, I do care, so here's the diff I would propose. A test compile
with my old GCC-2.8.1 cross-compiler on an Alpha/Tru64 box had no
complaints.

Index: strtol.c
===================================================================
RCS file: /cvs/djgpp/djgpp/src/libc/ansi/stdlib/strtol.c,v
retrieving revision 1.4
diff -u -p -3 -r1.4 strtol.c
--- strtol.c	2001/02/28 20:55:12	1.4
+++ strtol.c	2001/03/01 13:34:40
@@ -9,9 +9,9 @@
 long
 strtol(const char *nptr, char **endptr, int base)
 {
-  const char *s = nptr;
+  const unsigned char *s = (const unsigned char *) nptr;
   unsigned long acc;
-  int c;
+  unsigned char c;
   unsigned long cutoff;
   int neg = 0, any, cutlim;
 
@@ -22,7 +22,7 @@ strtol(const char *nptr, char **endptr, 
    */
   do {
     c = *s++;
-  } while (isspace(c & 0xff));
+  } while (isspace(c));
   if (c == '-')
   {
     neg = 1;
@@ -60,7 +60,7 @@ strtol(const char *nptr, char **endptr, 
   cutoff = neg ? -(unsigned long)LONG_MIN : LONG_MAX;
   cutlim = cutoff % (unsigned long)base;
   cutoff /= (unsigned long)base;
-  for (acc = 0, any = 0, c &= 0xff;; c = *s++, c &= 0xff)
+  for (acc = 0, any = 0; ; c = *s++)
   {
     if (isdigit(c))
       c -= '0';

-- 
Hans-Bernhard Broeker (broeker AT physik DOT rwth-aachen DOT de)
Even if all the snow were burnt, ashes would remain.

- Raw text -