-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Stream::readBytesUntil() with non-ASCII terminator #7053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
cvariable, 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 newintvariable that gets assigned toconce it is checked that it is non-negative.
- 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
cvariable
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?
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.
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?
Better to amend, that keeps the history cleaner after merging.
Allow Stream::readBytesUntil() and Stream::readStringUntil() use a negative value(i.e. a non-ASCII char) as a terminator. Replaces `[int] == [char]` comparisons with `(char)[int] == [char]`. Solves #1148
45232b2 to
6c6f538
Compare
CLA assistant check
All committers have signed the CLA.
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)