Stream::readBytesUntil() with non-ASCII terminator

[cousteaulecommandant]

Allow Stream::readBytesUntil() and Stream::readStringUntil() use a negative value as a terminator (i.e. a non-ASCII char).
Replaces [int] == [char] comparisons with [int] == (unsigned char)[char].
This should solve #1148, although I admit I haven't tested it.

(I was bored and decided to solve old issues)

[matthijskooijman]

Looks like a reasonable change. However, two questions:

• Won't this give a signed vs unsigned comparison warning? Or is that only for >, <, etc.?
• Would it not be sensible to convert c to char instead? Value > 127 should wrap around properly (i.e. it just truncates the bits, no other fancyness happens in that conversion). Then you'd have the same type on both ends of the comparison, which seems less error prone to me (and solve the warning I mentioned, if it occurs).
• If you do cast c to char, it might be useful to just change the type of the c variable, to prevent later changes that forget to cast it. Since you do need the extra range of the int in the < 0 check, you'd need to introduce a new int variable that gets assigned to c once it is checked that it is non-negative.

[cousteaulecommandant]

• Won't this give a signed vs unsigned comparison warning? Or is that only for >, <, etc.?

No because in this case we're comparing an int with an unsigned char, and thus the unsigned char gets promoted to int before comparing, and since the range of unsigned char fits in the range of int the value won't get affected.
If it were a comparison between int and unsigned int then yes; there would be side effects and the compiler will warn if -Wall is enabled (which it isn't by default, unfortunately).

• Would it not be sensible to convert c to char instead?

Maybe. This was more of a superstition thing to be honest (out-of-range conversion to a signed type is not undefined behavior, but other out-of-range issues are so I generally prefer to play it safe), but maybe you're right that it makes more sense to compare two chars.
Another reason was that functions like getchar() "return the character as an unsigned char cast to int", so I was just doing the same thing to the other side of the ==.

• If you do cast c to char, it might be useful to just change the type of the c variable

I'd rather add a cast than create an extra variable (which may or may not get optimized away); it feels like a smaller and simpler change, don't you think?

[matthijskooijman]

I'd rather add a cast than create an extra variable (which may or may not get optimized away); it feels like a smaller and simpler change, don't you think?

It's certainly simpler, yes. I don't really have a strong preference either way.

[cousteaulecommandant]

OK, then I think I'll just replace the (unsigned char) cast with the (char) cast... should I amend the commit, or just add a new one?

[matthijskooijman]

Better to amend, that keeps the history cleaner after merging.

[CLAassistant]

All committers have signed the CLA.