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
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.
Hi Andy,
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) {
Often in these cases, there's some predefined constant that should be used in lieu of -1. You introduced such a change in the same patch: - if (SetFilePointer(pSubjectInfo->hFile, 0, NULL, SEEK_END) == -1) + if (SetFilePointer(pSubjectInfo->hFile, 0, NULL, SEEK_END) == INVALID_SET_FILE_POINTER)
This sort of change makes the code better, in my opinion, because the return type is being compared against something defined in the interface's documentation.
The case I objected to is a curious one. I had a look at K&R's type promotion rules (2nd edition, section A6.5) and I'm confused what the compiler is doing here. The if-block is:
if (pbEncoded[1] + 1 > cbEncoded)
Rewriting the parenthesized expression as types rather than variables/values yields:
unsigned char + int > unsigned int
According to K&R, the unsigned char should get promoted to unsigned int (I think), hence:
unsigned int + int > unsigned int
When an unsigned type and a signed type are together in an expression, the signed type is promoted to unsigned:
unsigned int + unsigned int > unsigned int
Thus the comparison shouldn't have a signed/unsigned mismatch.
The promotion of unsigned char to signed/unsigned int is platform-dependent, so perhaps gcc's analysis assumes it's signed? In any case, by my reading the gcc warning is simply wrong. I could be wrong of course.
Anyway, good luck with your warning-crushing war ;-) --Juan
Hi Juan,
Juan Lang wrote:
The case I objected to is a curious one. I had a look at K&R's type promotion rules (2nd edition, section A6.5) and I'm confused what the compiler is doing here. The if-block is:
if (pbEncoded[1] + 1 > cbEncoded)
Rewriting the parenthesized expression as types rather than variables/values yields:
unsigned char + int > unsigned int
According to K&R, the unsigned char should get promoted to unsigned int (I think), hence:
unsigned int + int > unsigned int
Actually, I think it is true to say that "integral promotion" is first applied to each operand of the addition, giving:
int + int > unsigned int
since an int is large enough to hold all possible values of an unsigned char. So the warning is correct, since, in the general case, bad things would happen if the left-hand sum were to produce a negative value.
When an unsigned type and a signed type are together in an expression, the signed type is promoted to unsigned:
unsigned int + unsigned int > unsigned int
True, unless the signed operand has more bits than the unsigned one (e.g. on a system with 32-bit intS and 64-bit longS). The "usual arithmetic conversions" are rather complicated, but, in our example, the int sum will get promoted to an unsigned int. So here
int > unsigned int
becomes
unsigned int > unsigned int
An example of where this could go wrong is where the comparison (-1 > 0) [should be false] might be evaluated as (UINT_MAX > 0) [always true].
The promotion of unsigned char to signed/unsigned int is platform-dependent, so perhaps gcc's analysis assumes it's signed? In any case, by my reading the gcc warning is simply wrong. I could be wrong of course.
Integral promotion dictates that an anything whose type is of lower rank than an int will be promoted to an int if that is big enough to hold all possible values of the original type, else it will be promoted to an unsigned int. So an unsigned char (always eight bits wide) will always promote to an int (always >= sixteen bits wide). But the promotion of unsigned short to signed/unsigned int will depend on the relative widths involved, so will be platform dependent, as will the usual arithmetic conversion of int to signed/unsigned long.
(I hope I managed to get all of that right.)
Anyway, good luck with your warning-crushing war ;-) --Juan
Thank you and keep up the good work, too!
-
Andrew Talbot -
Juan Lang