[v8 PATCH] oleaut32: Improve support for IDispatch in VarCat
Jacek Caban
jacek at codeweavers.com
Wed May 17 06:06:53 CDT 2017
Hi Alistair,
On 12.05.2017 05:56, Alistair Leslie-Hughes wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=27106
>
> v6 - Simplify the code when the Variant cannot be coerced.
> v7 - Fix copy/paste bug in return value.
> v8 - Fixed failed test.
>
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair at hotmail.com>
> ---
> dlls/oleaut32/tests/vartest.c | 217 ++++++++++++++++++++++++++++++++++++++++++
> dlls/oleaut32/variant.c | 52 +++-------
> 2 files changed, 229 insertions(+), 40 deletions(-)
>
> diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c
> index 0e86ffb..f4224dc 100644
> --- a/dlls/oleaut32/tests/vartest.c
> +++ b/dlls/oleaut32/tests/vartest.c
> @@ -97,6 +97,150 @@ static BOOL has_i8;
> #define R8_MAX DBL_MAX
> #define R8_MIN DBL_MIN
>
> +#define DEFINE_EXPECT(func) \
> + static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
> +
> +#define SET_EXPECT(func) \
> + do { called_ ## func = FALSE; expect_ ## func = TRUE; } while(0)
> +
> +#define CHECK_EXPECT2(func) \
> + do { \
> + ok(expect_ ##func, "unexpected call " #func "\n"); \
> + called_ ## func = TRUE; \
> + }while(0)
> +
> +#define CHECK_EXPECT(func) \
> + do { \
> + CHECK_EXPECT2(func); \
> + expect_ ## func = FALSE; \
> + }while(0)
> +
> +#define CHECK_CALLED(func) \
> + do { \
> + ok(called_ ## func, "expected " #func "\n"); \
> + expect_ ## func = called_ ## func = FALSE; \
> + }while(0)
> +
> +DEFINE_EXPECT(dispatch_invoke);
> +
> +typedef struct
> +{
> + IDispatch IDispatch_iface;
> + VARTYPE vt;
> + HRESULT result;
> +} DummyDispatch;
> +
> +static inline DummyDispatch *impl_from_IDispatch(IDispatch *iface)
> +{
> + return CONTAINING_RECORD(iface, DummyDispatch, IDispatch_iface);
> +}
> +
> +static ULONG WINAPI DummyDispatch_AddRef(IDispatch *iface)
> +{
> + return 2;
> +}
> +
> +static ULONG WINAPI DummyDispatch_Release(IDispatch *iface)
> +{
> + return 1;
> +}
> +
> +static HRESULT WINAPI DummyDispatch_QueryInterface(IDispatch *iface,
> + REFIID riid,
> + void** ppvObject)
> +{
> + *ppvObject = NULL;
> +
> + if (IsEqualIID(riid, &IID_IDispatch) ||
> + IsEqualIID(riid, &IID_IUnknown))
> + {
> + *ppvObject = iface;
> + IDispatch_AddRef(iface);
> + }
> +
> + return *ppvObject ? S_OK : E_NOINTERFACE;
> +}
> +
> +static HRESULT WINAPI DummyDispatch_GetTypeInfoCount(IDispatch *iface, UINT *pctinfo)
> +{
> + ok(0, "Unexpected call\n");
> + return E_NOTIMPL;
> +}
> +
> +static HRESULT WINAPI DummyDispatch_GetTypeInfo(IDispatch *iface, UINT tinfo, LCID lcid, ITypeInfo **ti)
> +{
> + ok(0, "Unexpected call\n");
> + return E_NOTIMPL;
> +}
> +
> +static HRESULT WINAPI DummyDispatch_GetIDsOfNames(IDispatch *iface, REFIID riid, LPOLESTR *names,
> + UINT cnames, LCID lcid, DISPID *dispid)
> +{
> + ok(0, "Unexpected call\n");
> + return E_NOTIMPL;
> +}
> +
> +static HRESULT WINAPI DummyDispatch_Invoke(IDispatch *iface,
> + DISPID dispid, REFIID riid,
> + LCID lcid, WORD wFlags,
> + DISPPARAMS *params,
> + VARIANT *res,
> + EXCEPINFO *ei,
> + UINT *arg_err)
> +{
> + DummyDispatch *This = impl_from_IDispatch(iface);
> +
> + CHECK_EXPECT(dispatch_invoke);
> +
> + ok(dispid == DISPID_VALUE, "got dispid %d\n", dispid);
> + ok(IsEqualIID(riid, &IID_NULL), "go riid %s\n", wine_dbgstr_guid(riid));
> + ok(wFlags == DISPATCH_PROPERTYGET, "Flags wrong\n");
> +
> + ok(params->rgvarg == NULL, "got %p\n", params->rgvarg);
> + ok(params->rgdispidNamedArgs == NULL, "got %p\n", params->rgdispidNamedArgs);
> + ok(params->cArgs == 0, "got %d\n", params->cArgs);
> + ok(params->cNamedArgs == 0, "got %d\n", params->cNamedArgs);
> +
> + ok(res != NULL, "got %p\n", res);
> + ok(V_VT(res) == VT_EMPTY, "got %d\n", V_VT(res));
> + ok(ei == NULL, "got %p\n", ei);
> + ok(arg_err == NULL, "got %p\n", arg_err);
> +
> + if (FAILED(This->result))
> + return This->result;
> +
> + V_VT(res) = This->vt;
> + if (This->vt == VT_UI1)
> + V_UI1(res) = 34;
> + else if (This->vt == VT_NULL)
> + {
> + V_VT(res) = VT_NULL;
> + V_BSTR(res) = NULL;
> + }
> + else
> + memset(res, 0, sizeof(*res));
> +
> + return S_OK;
> +}
> +
> +static const IDispatchVtbl DummyDispatch_VTable =
> +{
> + DummyDispatch_QueryInterface,
> + DummyDispatch_AddRef,
> + DummyDispatch_Release,
> + DummyDispatch_GetTypeInfoCount,
> + DummyDispatch_GetTypeInfo,
> + DummyDispatch_GetIDsOfNames,
> + DummyDispatch_Invoke
> +};
> +
> +static void init_test_dispatch(VARTYPE vt, DummyDispatch *dispatch)
> +{
> + dispatch->IDispatch_iface.lpVtbl = &DummyDispatch_VTable;
> + dispatch->vt = vt;
> + dispatch->result = S_OK;
> +}
> +
> typedef struct IRecordInfoImpl
> {
> IRecordInfo IRecordInfo_iface;
> @@ -5607,6 +5751,7 @@ static void test_VarCat(void)
> HRESULT hres;
> HRESULT expected_error_num;
> int cmp;
> + DummyDispatch dispatch;
>
> CHECKPTR(VarCat);
>
> @@ -5941,6 +6086,78 @@ static void test_VarCat(void)
> VariantClear(&result);
> VariantClear(&expected);
>
> + /* Dispatch conversion */
> + init_test_dispatch(VT_NULL, &dispatch);
> + V_VT(&left) = VT_DISPATCH;
> + V_DISPATCH(&left) = &dispatch.IDispatch_iface;
> +
> + V_VT(&expected) = VT_BSTR;
> + V_BSTR(&expected)= SysAllocString(sz_empty);
> +
> + SET_EXPECT(dispatch_invoke);
> + hres = VarCat(&left, &right, &result);
> + ok(hres == S_OK, "got 0x%08x\n", hres);
> + ok(V_VT(&result) == VT_BSTR, "got %d\n", V_VT(&result));
> + ok(SysStringLen(V_BSTR(&result)) == 0, "got %d\n", SysStringLen(V_BSTR(&result)));
> + CHECK_CALLED(dispatch_invoke);
> + ok(VarCmp(&result,&expected,lcid,0) == VARCMP_EQ, "VarCat: DISPATCH concat with EMPTY did not return empty VT_BSTR\n");
This VarCmp doesn't test any more than you tested a few lines earlier.
I'd remove it.
> +
> + VariantClear(&left);
> + VariantClear(&right);
> + VariantClear(&result);
> + VariantClear(&expected);
> +
> + init_test_dispatch(VT_NULL, &dispatch);
> + V_VT(&right) = VT_DISPATCH;
> + V_DISPATCH(&right) = &dispatch.IDispatch_iface;
> +
> + SET_EXPECT(dispatch_invoke);
> + hres = VarCat(&left, &right, &result);
> + ok(hres == S_OK, "got 0x%08x\n", hres);
> + ok(V_VT(&result) == VT_BSTR, "got %d\n", V_VT(&result));
> + ok(SysStringLen(V_BSTR(&result)) == 0, "got %d\n", SysStringLen(V_BSTR(&result)));
> + CHECK_CALLED(dispatch_invoke);
> + ok(VarCmp(&result,&expected,lcid,0) == VARCMP_EQ, "VarCat: DISPATCH concat with EMPTY did not return empty VT_BSTR\n");
Same here.
> +
> + VariantClear(&left);
> + VariantClear(&right);
> + VariantClear(&result);
> + VariantClear(&expected);
> +
> + init_test_dispatch(VT_UI1, &dispatch);
> + V_VT(&right) = VT_DISPATCH;
> + V_DISPATCH(&right) = &dispatch.IDispatch_iface;
> +
> + V_VT(&expected) = VT_BSTR;
> + V_VT(&left) = VT_BSTR;
> + V_BSTR(&left) = SysAllocString(sz12);
> + V_BSTR(&expected) = SysAllocString(sz1234);
> + SET_EXPECT(dispatch_invoke);
> + hres = pVarCat(&left,&right,&result);
> + ok(hres == S_OK, "VarCat failed with error 0x%08x\n", hres);
> + CHECK_CALLED(dispatch_invoke);
> + ok(VarCmp(&result,&expected,lcid,0) == VARCMP_EQ, "VarCat: VT_DISPATCH concat with VT_BSTR failed to return correct result\n");
I'd replace that with explicit result tests, using strcmp_wa().
> +
> + VariantClear(&left);
> + VariantClear(&right);
> + VariantClear(&result);
> + VariantClear(&expected);
> +
> + init_test_dispatch(VT_NULL, &dispatch);
> + dispatch.result = E_OUTOFMEMORY;
> + V_VT(&right) = VT_DISPATCH;
> + V_DISPATCH(&right) = &dispatch.IDispatch_iface;
> +
> + SET_EXPECT(dispatch_invoke);
> + hres = VarCat(&left, &right, &result);
> + todo_wine ok(hres == E_OUTOFMEMORY, "got 0x%08x\n", hres);
> + CHECK_CALLED(dispatch_invoke);
This test shows that there is still something wrong with error handling
with your patch.
> +
> + VariantClear(&left);
> + VariantClear(&right);
> + VariantClear(&result);
> + VariantClear(&expected);
> +
> /* Test boolean conversion */
> V_VT(&left) = VT_BOOL;
> V_BOOL(&left) = VARIANT_TRUE;
> diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c
> index 17f753a..aa86be0 100644
> --- a/dlls/oleaut32/variant.c
> +++ b/dlls/oleaut32/variant.c
> @@ -2605,30 +2605,16 @@ HRESULT WINAPI VarCat(LPVARIANT left, LPVARIANT right, LPVARIANT out)
> else
> V_BSTR(&bstrvar_left) = SysAllocString(str_false);
> }
Note that V_BOOL case could also be handled by VariantChangeTypeEx if
you used VARIANT_ALPHABOOL flag.
> - /* Fill with empty string for later concat with right side */
> - else if (leftvt == VT_NULL)
> - {
> - V_VT(&bstrvar_left) = VT_BSTR;
> - V_BSTR(&bstrvar_left) = SysAllocString(sz_empty);
> - }
> else
> {
> + /* The value may not be able to be coerced, ie. VT_NULL */
> hres = VariantChangeTypeEx(&bstrvar_left,left,0,0,VT_BSTR);
> - if (hres != S_OK) {
> - VariantClear(&bstrvar_left);
> - VariantClear(&bstrvar_right);
> - if (leftvt == VT_NULL && (rightvt == VT_EMPTY ||
> - rightvt == VT_NULL || rightvt == VT_I2 ||
> - rightvt == VT_I4 || rightvt == VT_R4 ||
> - rightvt == VT_R8 || rightvt == VT_CY ||
> - rightvt == VT_DATE || rightvt == VT_BSTR ||
> - rightvt == VT_BOOL || rightvt == VT_DECIMAL ||
> - rightvt == VT_I1 || rightvt == VT_UI1 ||
> - rightvt == VT_UI2 || rightvt == VT_UI4 ||
> - rightvt == VT_I8 || rightvt == VT_UI8 ||
> - rightvt == VT_INT || rightvt == VT_UINT))
> - return DISP_E_BADVARTYPE;
Removing dead code should be a separated patch.
> + if (hres != S_OK && hres != DISP_E_TYPEMISMATCH)
> return hres;
> + else if(FAILED(hres))
> + {
> + V_VT(&bstrvar_left) = VT_BSTR;
> + V_BSTR(&bstrvar_left) = SysAllocString(sz_empty);
> }
Something like:
if (hres == DISP_E_TYPEMISMATCH) {
// set empty string
}else if(hres != S_OK)
return hres;
would be cleaner. However, more importantly, this needs more tests.
There are more cases than IDispatch when your patch changes behaviour,
so we need to make sure what behaviour is desired first.
> }
> }
> @@ -2645,30 +2631,16 @@ HRESULT WINAPI VarCat(LPVARIANT left, LPVARIANT right, LPVARIANT out)
> else
> V_BSTR(&bstrvar_right) = SysAllocString(str_false);
> }
> - /* Fill with empty string for later concat with right side */
> - else if (rightvt == VT_NULL)
> - {
> - V_VT(&bstrvar_right) = VT_BSTR;
> - V_BSTR(&bstrvar_right) = SysAllocString(sz_empty);
> - }
> else
> {
> + /* The value may not be able to be coerced, ie. VT_NULL */
> hres = VariantChangeTypeEx(&bstrvar_right,right,0,0,VT_BSTR);
> - if (hres != S_OK) {
> - VariantClear(&bstrvar_left);
> - VariantClear(&bstrvar_right);
> - if (rightvt == VT_NULL && (leftvt == VT_EMPTY ||
> - leftvt == VT_NULL || leftvt == VT_I2 ||
> - leftvt == VT_I4 || leftvt == VT_R4 ||
> - leftvt == VT_R8 || leftvt == VT_CY ||
> - leftvt == VT_DATE || leftvt == VT_BSTR ||
> - leftvt == VT_BOOL || leftvt == VT_DECIMAL ||
> - leftvt == VT_I1 || leftvt == VT_UI1 ||
> - leftvt == VT_UI2 || leftvt == VT_UI4 ||
> - leftvt == VT_I8 || leftvt == VT_UI8 ||
> - leftvt == VT_INT || leftvt == VT_UINT))
> - return DISP_E_BADVARTYPE;
> + if (hres != S_OK && hres != DISP_E_TYPEMISMATCH)
> return hres;
> + else if(FAILED(hres))
> + {
> + V_VT(&bstrvar_right) = VT_BSTR;
> + V_BSTR(&bstrvar_right) = SysAllocString(sz_empty);
> }
> }
> }
> -- 1.9.1
Same for those changes: remove dead code in separated patch, add tests
and consider using VARIANT_ALPHABOOL.
Thanks,
Jacek
More information about the wine-devel
mailing list