Re: scrrun: Implement IDictionary get_HashVal (try 3)

[Nikolay Sivov]

On 11/8/2012 12:31, Alistair Leslie-Hughes wrote:
> Hi,
> Thanks Piotr for your help.
>
> Changelog:
>       scrrun: Implement IDictionary get_HashVal
> +static LONG create_hash_val_float(float fval)
> +{
> - TRACE("val = %f\n", fval);
> -
> - if(isinf(fval))
> -
>    return 0;
> -
> - return (*((DWORD*)&fval)) % 1201;
>
> +}

Emm, do you just mean (DWORD)fval here? If you always hash it as DWORD 
why not use DWORD argument? And isinf() will stay for float types in 
your switch(...).
> - FIXME("(%p)->(%p %p)\n", This, Key, HashVal);
> - TRACE("(%p)->(%s %s)\n", This, debugstr_variant(Key), debugstr_variant(HashVal));

Tracing HashVal is not useful this way.
> -
>    case VT_R8:
> -
>        V_I4(HashVal) = create_hash_val_float(V_R8(Key));
> -
>        break;
> -
>    case VT_I4:
> -
>        V_I4(HashVal) = create_hash_val_float(V_I4(Key));
> -
>        break;

That's confusing, I mean the way you treat integer type here.

scrrun: Implement IDictionary get_HashVal (try 3)

[Piotr Caban]

On 11/8/2012 12:31, Alistair Leslie-Hughes wrote:
+static LONG create_hash_val(BSTR name)
+{
+    LONG hash = 0;
The create_hash_val function should do the computations on unsigned 
variable.

On 11/08/12 15:26, Nikolay Sivov wrote:
> On 11/8/2012 12:31, Alistair Leslie-Hughes wrote:
> > +static LONG create_hash_val_float(float fval)
> > +{
> > - TRACE("val = %f\n", fval);
> > -
> > - if(isinf(fval))
> > -
> >    return 0;
> > -
> > - return (*((DWORD*)&fval)) % 1201;
> >
> > +}
>
> Emm, do you just mean (DWORD)fval here? If you always hash it as DWORD
> why not use DWORD argument? And isinf() will stay for float types in
> your switch(...).

This code is operating on binary representation of float.

I haven't tested it but I guess NaN needs special handling as well.
> > -
> >    case VT_R8:
> > -
> >        V_I4(HashVal) = create_hash_val_float(V_R8(Key));
> > -
> >        break;
> > -
> >    case VT_I4:
> > -
> >        V_I4(HashVal) = create_hash_val_float(V_I4(Key));
> > -
> >        break;
>
> That's confusing, I mean the way you treat integer type here.

It's the way it's done in native dll. Thanks to it (float)11.0, 
(double)11.0 and (int)11 has the same hash value.

scrrun: Implement IDictionary get_HashVal (try 3)

[Nikolay Sivov]

On 11/8/2012 18:44, Piotr Caban wrote:
> On 11/8/2012 12:31, Alistair Leslie-Hughes wrote:
> +static LONG create_hash_val(BSTR name)
> +{
> - LONG hash = 0;
>
> The create_hash_val function should do the computations on unsigned 
> variable.
>
> On 11/08/12 15:26, Nikolay Sivov wrote:
> > On 11/8/2012 12:31, Alistair Leslie-Hughes wrote:
> > > +static LONG create_hash_val_float(float fval)
> > > +{
> > > - TRACE("val = %f\n", fval);
> > > -
> > > - if(isinf(fval))
> > > -
> > >    return 0;
> > > -
> > > - return (*((DWORD*)&fval)) % 1201;
> > >
> > > +}
> >
> > Emm, do you just mean (DWORD)fval here? If you always hash it as DWORD
> > why not use DWORD argument? And isinf() will stay for float types in
> > your switch(...).
>
> This code is operating on binary representation of float.
>
> I haven't tested it but I guess NaN needs special handling as well.

Sure. What I meant is to move type-specific handling outside of hash 
computation function and use argument type that doesn't depend on 
variant type, e.g. DWORD cause it casts to it already.
> > > -
> > >    case VT_R8:
> > > -
> > >        V_I4(HashVal) = create_hash_val_float(V_R8(Key));
> > > -
> > >        break;
> > > -
> > >    case VT_I4:
> > > -
> > >        V_I4(HashVal) = create_hash_val_float(V_I4(Key));
> > > -
> > >        break;
> >
> > That's confusing, I mean the way you treat integer type here.
>
> It's the way it's done in native dll. Thanks to it (float)11.0, 
> (double)11.0 and (int)11 has the same hash value.

Okay, so you mean float should be used as base type here.