Re: advpack: Sign-compare warnings fix (Resend)

[Michael Stefaniuc]

Andrew Talbot wrote:
> Is there anything wrong with this patch, please?

If I might venture a guess it's probably because of the casts. Casts are 
evil if not really needed and using casts to get rid of the sign 
comparison warnings is debatable at best.
> Changelog:
>     advpack: Sign-compare warnings fix.
>
> diff --git a/dlls/advpack/files.c b/dlls/advpack/files.c
> index 5e9ce30..43c07ea 100644
> --- a/dlls/advpack/files.c
> +++ b/dlls/advpack/files.c
> @@ -709,7 +709,7 @@ HRESULT WINAPI ExtractFilesA(LPCSTR CabName, LPCSTR ExpandDir, DWORD Flags,
>      if (FileList)
>      {
>          szConvertedList = convert_file_list(FileList, &dwFileCount);
> -
>    if (!szConvertedList || dwFileCount == -1)
> -
>    if (!szConvertedList || (LONG)dwFileCount == -1)

This one could be replaced by a comparison with either "-1u" or "~0".
>      {
>          res = E_FAIL;
>          goto done;
>
> diff --git a/dlls/advpack/install.c b/dlls/advpack/install.c
> index c5a5df0..05a7232 100644
> --- a/dlls/advpack/install.c
> +++ b/dlls/advpack/install.c
> @@ -41,7 +41,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(advpack);
>  #define SPAPI_MASK      0xFFFFL
>  #define HRESULT_FROM_SPAPI(x)   ((x & SPAPI_MASK) | SPAPI_PREFIX)
>
> -#define ADV_HRESULT(x)  ((x & SPAPI_ERROR) ? HRESULT_FROM_SPAPI(x) : HRESULT_FROM_WIN32(x))
> +#define ADV_HRESULT(x)  ((x & SPAPI_ERROR) ? HRESULT_FROM_SPAPI(x) : (DWORD)HRESULT_FROM_WIN32(x))
>
> #define ADV_SUCCESS     0
>  #define ADV_FAILURE     1

bye
    michael

advpack: Sign-compare warnings fix (Resend)

[Andrew Talbot]

Hi Michael,

Michael Stefaniuc wrote:
> Andrew Talbot wrote:
> > -
> >    if (!szConvertedList || dwFileCount == -1)
> > -
> >    if (!szConvertedList || (LONG)dwFileCount == -1)
>
> This one could be replaced by a comparison with either "-1u" or "~0".
>
> bye
> michael

The first of these would work; the second would not, since it just inverts
the bits of decimal zero, which is a signed int. (The reason I say "decimal
zero" is because decimal constants are signed, whereas hexadecimal constants
are unsigned[!] Thus, ~0x0 would be a viable alternative.)

Thanks and regards,

advpack: Sign-compare warnings fix (Resend)

[Andrew Talbot]

Andrew Talbot wrote:
> (The reason I say
> "decimal zero" is because decimal constants are signed, whereas
> hexadecimal constants are unsigned[!] Thus, ~0x0 would be a viable
> alternative.)

In fact, I have just tried both ~0 and ~0x0 and neither worked. (I can't figure out why the latter fails.) But -1u works, yes.

advpack: Sign-compare warnings fix (Resend)

[Andrew Talbot]

Andrew Talbot wrote:
> Andrew Talbot wrote:
> > (The reason I say
> > "decimal zero" is because decimal constants are signed, whereas
> > hexadecimal constants are unsigned[!] Thus, ~0x0 would be a viable
> > alternative.)
>
> In fact, I have just tried both ~0 and ~0x0 and neither worked. (I can't
> figure out why the latter fails.) But -1u works, yes.

Sorry for the noise, but just to say I was wrong in claiming that hex
constants are unsigned. I have opted for ~0U in try #2.

Thanks,

advpack: Sign-compare warnings fix (Resend)

[Michael Karcher]

Am Montag, den 14.07.2008, 23:33 +0100 schrieb Andrew Talbot:
> > > (The reason I say
> > > "decimal zero" is because decimal constants are signed, whereas
> > > hexadecimal constants are unsigned[!] Thus, ~0x0 would be a viable
> > > alternative.)
> >
> > In fact, I have just tried both ~0 and ~0x0 and neither worked. (I can't
> > figure out why the latter fails.) But -1u works, yes.
>
> Sorry for the noise, but just to say I was wrong in claiming that hex
> constants are unsigned. I have opted for ~0U in try #2.

Good choice. While I wanted to post that I like -1u much better, as I
feel it as the canonical choice for all-bits-set, a grep on the wine
source code showed that wine always uses ~0U, and never -1U, so in wine
it is ~0U (or ~0UL if dealing with platform longs).

Regards,
  Michael Karcher