Date: Thu, 1 Mar 2001 15:03:59 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: Hans-Bernhard Broeker cc: djgpp workers list Subject: Re: strtol fix correct? In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 Precedence: bulk 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.