Hi Andy,
+ LONG last_error;
ret = CryptGetObjectUrl(URL_OID_CERTIFICATE_CRL_DIST_POINT, rgpvContext[i], 0, NULL, &cbUrlArray, NULL, NULL, NULL); - if (!ret && GetLastError() == CRYPT_E_NOT_FOUND) + last_error = GetLastError(); + if (!ret && last_error == CRYPT_E_NOT_FOUND)
I don't get it. How does using a temporary LONG rather than GetLastError directly remove the warning? Either the compiler was wrong about the first warning, or it's wrong about not warning about this form. I don't see the utility of this patch. Furthermore, any patches that remove warnings due to sign mismatches between GetLastError and an error in winerror.h reflect an error on Microsoft's part, not on Wine's, so I don't see the point of removing them. --Juan
Juan Lang wrote:
Hi Andy,
LONG last_error; ret = CryptGetObjectUrl(URL_OID_CERTIFICATE_CRL_DIST_POINT, rgpvContext[i], 0, NULL, &cbUrlArray, NULL, NULL, NULL);
if (!ret && GetLastError() == CRYPT_E_NOT_FOUND)
last_error = GetLastError();if (!ret && last_error == CRYPT_E_NOT_FOUND)I don't get it. How does using a temporary LONG rather than GetLastError directly remove the warning? Either the compiler was wrong about the first warning, or it's wrong about not warning about this form. I don't see the utility of this patch. Furthermore, any patches that remove warnings due to sign mismatches between GetLastError and an error in winerror.h reflect an error on Microsoft's part, not on Wine's, so I don't see the point of removing them. --Juan
Hi Juan,
An error code like CRYPT_E_NOT_FOUND is, in essence, a LONG (signed). Whereas GetLastError() returns a DWORD (unsigned, which does seem inappropriate to me). So the compiler is correct to warn about the original code. In the case of the assignment to the temporary, the compiler automatically coerces the right-hand side to suit the left. Let me attempt to illustrate with the following code.
signed char i, j, k;
i = j + k;
In the above code, j and k would first be automatically promoted to intS (integer promotion), then they would be added together, the result being an int. Finally, this result would be coerced back into being a signed char again: no cast required. (It's up to the programmer to avoid overflow). Thus, one can avoid a cast by using an assignment.
Regarding whether there is a point in removing such errors: I suppose, if we want to apply -Wsign-compare for the whole codebase and have zero warnings during a build, then I don't think we have any choice.
Hi Andy,
An error code like CRYPT_E_NOT_FOUND is, in essence, a LONG (signed). Whereas GetLastError() returns a DWORD (unsigned, which does seem inappropriate to me).
Yes, I know what the value of CRYPT_E_NOT_FOUND is, and what the type of GetLastError is. My point is, Microsoft confused signed and unsigned types for their last error values. We have to live with it.
Regarding whether there is a point in removing such errors: I suppose, if we want to apply -Wsign-compare for the whole codebase and have zero warnings during a build, then I don't think we have any choice.
I don't think the cost of adding silly casts or temporaries to avoid giving a warning on a common code idiom is worth it. --Juan
Juan Lang wrote:
Yes, I know what the value of CRYPT_E_NOT_FOUND is, and what the type of GetLastError is. My point is, Microsoft confused signed and unsigned types for their last error values. We have to live with it.
Indeed. (And sorry, I didn't mean to sound patronizing.)
I don't think the cost of adding silly casts or temporaries to avoid giving a warning on a common code idiom is worth it.
That's the trouble with some warnings: some instances you want to fix and some you know you can safely ignore. I presumed that we were aiming to turn this warning on permanently, but that may not be the intention, and maybe the downside you highlight makes doing so not worthwhile.
That's the trouble with some warnings: some instances you want to fix and some you know you can safely ignore. I presumed that we were aiming to turn this warning on permanently, but that may not be the intention, and maybe the downside you highlight makes doing so not worthwhile.
Turning on -Wsign-compare may be useful, but I don't think combining it with -Werror is. This is the trouble with warnings in general: they may or may not point out a real bug. Fixing the bugs is useful, but fixing non-bugs really isn't, unless the code is somehow improved as a result. Making code look worse to remove a warning is clearly not useful. --Juan
-
Andrew Talbot -
Juan Lang