-
Notifications
You must be signed in to change notification settings - Fork 41
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
Unnecessary check in jsonsl_util_unescape_ex #35
Conversation
By the time JSONSL checks if a u-escape value is less than 0xDC00, it already knows it is greater than 0xD7FF.
Hrm. Should we just delete this branch? If this code was dead until now, then removing this branch would modify behavior. Can you think of some test where this would be true? |
I guess the issue here is just a superfluous check.
Of course, if one were to change the behavior of |
I'm slow :( |
... and -1 is handled above these conditions... |
I think the change is correct - that is, the change has no effect on the behavior, because the check that I'm removing is always true. The logic branch as a whole is still useful for any value from 0xD800 and 0xDC00 inclusive. Here's a summary of the old code:
In short, deleting the particular clause This branch is tested in a few different ways in
I just stepped through this test to ensure it hits the branch I changed, no matter whether I keep the unneeded test or not. Anyway, this doesn't really matter! =) If it's controversial then we can close this PR. My sole motivation is to resolve a Coverity warning in libbson, while keeping libbson's copy of JSONSL in sync with this repository. Alternatively I can just tell Coverity to ignore this warning. |
@mnunberg yeah please ignore me. Unless someone adds -2 as possible |
I chanced upon this: http://unicodebook.readthedocs.io/unicode_encodings.html#utf-16-surrogate-pairs If a unit (defined as 16 bits) falls within the surrogate range (0xD800-0xDBFF, 0xDC00-0xDFFF) then it must follow surrogate encoding rules. In other words, a character in those ranges must be a high surrogate without anything preceding it, or a low surrogate with a high surrogate preceding it. The check I'll merge this. I was mistakenly assuming that the branch was dead, but it's only part of the check which is superfluous. |
Thanks to both of you! |
Found by a Coverity scan. The Coverity analysis is: