Juan Lang wrote:
Hi Andy,
if (pbEncoded[1] + 1 > cbEncoded)
if (pbEncoded[1] + 1U > cbEncoded)Is this change necessary? The resulting code is less clear than the original, IMO. It's clearly a spurious warning: a BYTE (max value 255) + 1 can't yield a value that overflows an unsigned int, so this comparison will always do what's wanted.
Same with the change here:
else if (lenLen + 2 > cbEncoded)
else if (lenLen + 2U > cbEncoded)Otherwise, this patch looks fine to me. --Juan
Hi Juan,
I was curious to see how this one would fly. I fully take your point, of course. If it were a good idea, the point would be to reduce the noise when looking for real sign-compare problems and without introducing a cast. In a similar vein, quite a lot of warnings are generated by code like this
if (dwSomething == -1) { ...
Maybe, in this case the code actually is "wrong", since a variable that operates in the range 0 to UINT_MAX (on a non-MSC system) can never be strictly equal to -1, but in these cases I have resisted comparing against "~0U", because there may be places where an internal function could/should be rewritten not to use "minus one unsigned" as a return value, for example, and this "fix" would tend to hide those opportunities.
I shall see how the patch fares and resubmit without the "U"s if it gets rejected.
Thanks for the feedback.