Date:	Thu, 1 Mar 2001 15:03:59 +0200 (IST)
From:	Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
X-Sender:	eliz AT is
To:	Hans-Bernhard Broeker <broeker AT physik DOT rwth-aachen DOT de>
cc:	djgpp workers list <djgpp-workers AT delorie DOT com>
Subject:	Re: strtol fix correct?
In-Reply-To:	<Pine.LNX.4.10.10103011327160.8909-100000@acp3bf>
Message-ID:	<Pine.SUN.3.91.1010301145132.6488B-100000@is>
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, Hans-Bernhard Broeker wrote:

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

We've been through this before, when v2.03 was in the last stages of 
preparations for a release.  The funky "&= 0xff" were introduced in 
response to subtle bugs found very close to releasing v2.03.  While not 
very clean, these changes are localized, get the job done, and don't 
trigger warnings from the compiler, especially given the paranoiac set of 
warning switches and -Werror used by the library build procedure.

Changing the the declarations to unsigned char triggers compiler warnings 
about mismatch between signed and unsigned char, and trying to fix that 
tends to create a chain-reaction effect, whereby you are forced to 
declare more and more variables unsigned.  This can also create real 
bugs, because mixing signed and unsigned types is asking for trouble.

It didn't make sense at the time to risk breaking a perfectly healthy 
library just to be a bit cleaner.  In any case, where changing 
declarations to unsigned char was safe and localized, it was done.

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

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

- Raw text -