[PATCH] oleaut32: Improve support for IDispatch in VarCat

[Jacek Caban]

Hi Alistair,

On 17.04.2017 05:43, Alistair Leslie-Hughes wrote:
> diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c
> index 17f753af70..6f695c3f37 100644
> --- a/dlls/oleaut32/variant.c
> +++ b/dlls/oleaut32/variant.c
> @@ -2611,6 +2611,13 @@ HRESULT WINAPI VarCat(LPVARIANT left, LPVARIANT right, LPVARIANT out)
>                  V_VT(&bstrvar_left) = VT_BSTR;
>                  V_BSTR(&bstrvar_left) = SysAllocString(sz_empty);
>              }
> +            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);
> @@ -2651,6 +2658,13 @@ HRESULT WINAPI VarCat(LPVARIANT left, LPVARIANT right, LPVARIANT out)
>                  V_VT(&bstrvar_right) = VT_BSTR;
>                  V_BSTR(&bstrvar_right) = SysAllocString(sz_empty);
>              }
> +            else if (rightvt == VT_DISPATCH)
> +            {
> +                /* The returned Dispatch Value, may not be able to be coerced, ie. VT_NULL */
> +                hres = VariantChangeTypeEx(&bstrvar_right,right,0,0,VT_BSTR);
> +                if (hres != S_OK && hres != DISP_E_TYPEMISMATCH)
> +                    return hres;
> +            }
>              else
>              {
>                  hres = VariantChangeTypeEx(&bstrvar_right,right,0,0,VT_BSTR);

While this change may do the right thing in the case you're fixing, the
whole surrounding code looks just wrong and I don't like the idea of
making it even more complicated to work around problem in this single
case. For example take a look at if() statement in line 2620. It's never
executed, because it requires leftvt to be VT_NULL, which is handled
elsewhere (line 2609). If you remove that if(), this call is almost
identical to what you add here as a special case. The only difference is
DISP_E_TYPEMISMATCH handling. Did you investigate what happens in such
case? Maybe we should handle VariantChangeTypeEx failures like you do in
the patch?

Thanks,
Jacek