Use more correct code for compilers with signed chars

[gwsw]

The current code often stores file input and tty input in a char or array of chars, and converts each char to an int by ANDing with 0xFF. It would be more correct to store the raw input in unsigned chars.

[gwsw]

I made a preliminary attempt to implement this change. I mostly completed the work on 4 of the 37 source files. This resulted in changing about 1300 lines, and introducing about 75 ugly casts, mostly between unsigned char* and char* in order to pass a string to a library function.

I don't think this is a viable approach. I think instead I should leave the code as it stands, and just change the handful of places that convert a char to an unsigned char via ch & 0xFF to a cast (unsigned char) ch.

[avih]

I mostly completed the work on 4 of the 37 source files. This resulted in changing about 1300 lines, and introducing about 75 ugly casts

I don't think this is a viable approach

I agree. It feels too much for the cause.

I didn't look at the code yet, but I think the biggest value is only where such values are used as bytes, and I'm guessing these are not all over the place? while places which simply pass along char* strings or doing libc string operations, like strstr etc, don't really need touching.

Would that reduce the impacted LOC significantly?

[gwsw]

The change I attempted was to make every variable that stores a character be unsigned char rather than char (and likewise for arrays of chars and pointers to chars). That didn't work out. Yes, the real issue arises when we want to get the byte value of a char. In most cases today we do that by ANDing with 0xFF, which as you pointed out is not portable. So I'm proposing just changing those lines to a cast to unsigned char. I think there are fewer than 10 such lines in the whole code base, so yeah, it would significantly reduce the impacted lines. Maybe I'll make it a macro to make clear the purpose of the cast.

[avih]

So I'm proposing just changing those lines to a cast to unsigned char. I think there are fewer than 10 such lines in the whole code base, so yeah, it would significantly reduce the impacted lines.

That sounds much better to me. Post the patch here or as a PR before you push it?

[avih]

Also, buffers which are used as bytes sequences, like that utf8 thing you touched recently, are probably better as unsigned char buffers which are being worked on, and then simply cast to char* once the work with them is done (e.g. as return value or when calling other "normal" string functions)?

Basically, in functions which work on byte values, cast from char* to unsigned char* if needed early, and keep the internal stuff unsigned or otherwise suitable for these values, and only cast back to char* when the value leaves these functions.

I don't know how much that would affect the code. If it's too much, then maybe not, but if it's still relatively not too much, then it's probably worth it.

Also, do run your tests, which IIRC include unicode stuff, just in case.