Re: fusion: Explicitly check for -1 for a missing table

[Marcus Meissner]

On Wed, Apr 16, 2008 at 11:48:31AM -0500, James Hawkins wrote:
> Hi,
>
> Changelog:
> * Explicitly check for -1 for a missing table.
>
> dlls/fusion/assembly.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> -- 
> James Hawkins
> diff --git a/dlls/fusion/assembly.c b/dlls/fusion/assembly.c
> index 024e611..eee889a 100644
> --- a/dlls/fusion/assembly.c
> +++ b/dlls/fusion/assembly.c
> @@ -416,7 +416,7 @@ HRESULT assembly_get_name(ASSEMBLY *assembly, LPSTR *name)
>      ULONG offset;
>
> offset = assembly->tables[0x20].offset; /* FIXME: add constants */
> -    if (offset < 0)
> +    if (offset == -1)
>          return E_FAIL;
>
> asmtbl = (ASSEMBLYTABLE *)assembly_data_offset(assembly, offset);
> @@ -527,7 +527,7 @@ HRESULT assembly_get_pubkey_token(ASSEMBLY *assembly, LPSTR *token)
>      *token = NULL;
>
> offset = assembly->tables[0x20].offset; /* FIXME: add constants */
> -    if (offset < 0)
> +    if (offset == -1)
>          return E_FAIL;

You should make offset "signed", or pass errors down in another way.
This will not work this way.

Ciao, Marcus

fusion: Explicitly check for -1 for a missing table

[James Hawkins]

On Wed, Apr 16, 2008 at 11:51 AM, Marcus Meissner <meissner(a)suse.de> wrote:
> On Wed, Apr 16, 2008 at 11:48:31AM -0500, James Hawkins wrote:
> > Hi,
> >
> > Changelog:
> > * Explicitly check for -1 for a missing table.
> >
> > dlls/fusion/assembly.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > --
> > James Hawkins
> > diff --git a/dlls/fusion/assembly.c b/dlls/fusion/assembly.c
> > index 024e611..eee889a 100644
> > --- a/dlls/fusion/assembly.c
> > +++ b/dlls/fusion/assembly.c
> > @@ -416,7 +416,7 @@ HRESULT assembly_get_name(ASSEMBLY *assembly, LPSTR *name)
> >      ULONG offset;
> >
> > offset = assembly->tables[0x20].offset; /* FIXME: add constants */
> > -    if (offset < 0)
> > +    if (offset == -1)
> >          return E_FAIL;
> >
> > asmtbl = (ASSEMBLYTABLE *)assembly_data_offset(assembly, offset);
> > @@ -527,7 +527,7 @@ HRESULT assembly_get_pubkey_token(ASSEMBLY *assembly, LPSTR *token)
> >      *token = NULL;
> >
> > offset = assembly->tables[0x20].offset; /* FIXME: add constants */
> > -    if (offset < 0)
> > +    if (offset == -1)
> >          return E_FAIL;
>
> You should make offset "signed", or pass errors down in another way.
>  This will not work this way.

It works just fine.  -1 is 4294967295 in ULONG (32bit), which is
exactly the same as offset on error (because we assigned it -1, but
the representation in memory is the same).  We do this several other
places in the code base.

-- 
James Hawkins

fusion: Explicitly check for -1 for a missing table

[Robert Shearman]

James Hawkins wrote:
> It works just fine.  -1 is 4294967295 in ULONG (32bit), which is
> exactly the same as offset on error (because we assigned it -1, but
> the representation in memory is the same).

Then use ~0 so that the purpose is clearer. Adding a define for this 
value would probably further increase readability of the code.
> We do this several other
> places in the code base.

They should be changed too.

-- 
Rob Shearman