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.