X-Authentication-Warning: acp3bf.physik.rwth-aachen.de: broeker owned process doing -bs
Date: Thu, 1 Mar 2001 13:40:42 +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: strtol fix correct?
Message-ID: <Pine.LNX.4.10.10103011327160.8909-100000@acp3bf>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Reply-To: djgpp-workers AT delorie DOT com

Hello, everybody.

I just came across a posting by Eli to the DJGPP mailing list, regarding a
bugfix for strtol (subject was "atoi() with 8 bit chars").  Seeing all
those "&= 0xff" operations on a variable holding a character, and the call
of isdigit(c) rang an alarm bell, to me. This reeks of incorrect usage of
the is...() functions/macros from <ctype.h>, and a not really correct way
of fixing them. For reference, here's Eli's patch, again:

@@ -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 = *s++, c &= 0xff)
+  for (acc = 0, any = 0, c &= 0xff;; c = *s++, c &= 0xff)
   {
     if (isdigit(c))
       c -= '0';

In a nutshell: the argument passed to isdigit() and friends should always
be an unsigned char, or an integer holding an unsigned char's value. In
the patch hunk above, the variable 'c' is an int, but it's assigned
unmodified chars from the input string pointer 's', and that somewhat
spooky '&= 0xff' is meant to fix the problems this raises. 

If I'm not very much mistaken, it would be cleaner to change the type
of 'c' (and may be of 's', too) to unsigned char, and get rid of those
0xff's.

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++

	for (acc = 0, any = 0; ; c = (unsigned char) *s++)

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. 

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