Re: [PATCH] oleaut32: Improve support for IDispatch in VarCat

[Nikolay Sivov]

On 17.04.2017 6:43, Alistair Leslie-Hughes wrote:
> -
>        else if (rightvt == VT_DISPATCH)
> -
>        {
> -
>            /* The returned Dispatch Value, may not be able to be coerced, ie. VT_NULL */
> -
>            hres = VariantChangeTypeEx(&bstrvar_left,left,0,0,VT_BSTR);
> -
>            if (hres != S_OK && hres != DISP_E_TYPEMISMATCH)
> -
>                return hres;
> -
>        }
>        else
>        {
>            hres = VariantChangeTypeEx(&bstrvar_left,left,0,0,VT_BSTR);

This is suspiciously similar to default case, in last else block. Could
it mean VariantChangeTypeEx() should be fixed instead?
> - if (This->bFailInvoke)
> -
>    return E_OUTOFMEMORY;

I think it's better to use some non-specific error code that triggers
FAILED() but is not generic as E_OUTOFMEMORY, anything above 0x8000000 I
suppose. Also maybe it's better to use exact code instead of
bFailInvoke, and return if (FAILED(This->hr))?
> +static inline DummyDispatch *impl_from_IDispatch(IDispatch *iface)
> +{
> - return CONTAINING_RECORD(iface, DummyDispatch, IDispatch_iface);
>
> +}
> -
>
> +static ULONG WINAPI DummyDispatch_AddRef(IDispatch *iface)
> +{
> - DummyDispatch *This = impl_from_IDispatch(iface);
> - return InterlockedIncrement(&This->ref);
>
> +}

If static object is enough, you can remove reference counter.
> - if (pVarCmp)
> -
>    ok(pVarCmp(&result,&expected,lcid,0) == VARCMP_EQ,
> -
>       "VarCat: EMPTY concat with EMPTY did not return empty VT_BSTR\n");

This is not a problem this patch introduces, but there's no reason to
continue to use pVarCmp, as it's always available on platforms we're
testing on.

[PATCH] oleaut32: Improve support for IDispatch in VarCat

[Alistair Leslie-Hughes]

Hi Nikolay,
> This is suspiciously similar to default case, in last else block. Could
> it mean VariantChangeTypeEx() should be fixed instead?

I made it's own if, since I think it behaves different to the one below.  After more
testing, it appears all errors are treated as an "empty" value.

No, VariantChangeTypeEx is working as expected, and I'm in the process of 
writing tests to show this.

Best Regards
 Alistair Leslie-Hughes