Re: [PATCH] propsys: Add PropVariantToBoolean and tests

[Nikolay Sivov]

On 18.09.2017 21:20, Fabian Maurer wrote:
> Signed-off-by: Fabian Maurer
>
> dlls/propsys/propsys.spec    |   2 +-
>  dlls/propsys/propvar.c       |  58 +++++++++++++++++
>  dlls/propsys/tests/propsys.c | 147 +++++++++++++++++++++++++++++++++++++++++++
>  include/propvarutil.h        |   1 +
>  4 files changed, 207 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/propsys/propsys.spec b/dlls/propsys/propsys.spec
> index 582308f5a0..86a5e696eb 100644
> --- a/dlls/propsys/propsys.spec
> +++ b/dlls/propsys/propsys.spec
> @@ -107,7 +107,7 @@
>  @ stub PropVariantGetUInt32Elem
>  @ stub PropVariantGetUInt64Elem
>  @ stub PropVariantToBSTR
> -@ stub PropVariantToBoolean
> +@ stdcall PropVariantToBoolean(ptr ptr)
>  @ stub PropVariantToBooleanVector
>  @ stub PropVariantToBooleanVectorAlloc
>  @ stub PropVariantToBooleanWithDefault
> diff --git a/dlls/propsys/propvar.c b/dlls/propsys/propvar.c
> index ca3b421ddd..25cebfee02 100644
> --- a/dlls/propsys/propvar.c
> +++ b/dlls/propsys/propvar.c
> @@ -227,6 +227,64 @@ HRESULT WINAPI PropVariantToUInt64(REFPROPVARIANT propvarIn, ULONGLONG *ret)
>      return hr;
>  }
>
> +HRESULT WINAPI PropVariantToBoolean(REFPROPVARIANT propvarIn, BOOL *ret)
> +{
> - LONGLONG res;
> - HRESULT hr = S_OK;

Initialization does not do anything.
> - static const WCHAR wstr_true[] = {'t','r','u','e',0};
> - static const WCHAR wstr_false[] = {'f','a','l','s','e',0};
> - static const CHAR str_true[] = {'t','r','u','e',0};
> - static const CHAR str_false[] = {'f','a','l','s','e',0};

For single byte case you can use string literals right where you need them.
> -
> - TRACE("%p,%p\n", propvarIn, ret);
> -
> - *ret = 0;

Is it correct to crash on NULL argument?
> -
> - switch(propvarIn->vt)
> - {
> -
>    case VT_BOOL:
> -
>        *ret = propvarIn->u.boolVal == VARIANT_TRUE;
> -
>        return S_OK;
> -
> -
>    case VT_LPWSTR:
> -
>    case VT_BSTR:
> -
>        if(!propvarIn->u.pwszVal)
> -
>            return DISP_E_TYPEMISMATCH;
> -
> -
>        if(lstrcmpiW(propvarIn->u.pwszVal, wstr_true) == 0)
> -
>        {
> -
>            *ret = 1;
> -
>            return S_OK;
> -
>        }
> -
>        if(lstrcmpiW(propvarIn->u.pwszVal, wstr_false) == 0)
> -
>        {
> -
>            *ret = 0;
> -
>            return S_OK;
> -
>        }
> -
>        break;
> -

TRUE/FALSE are more common in this context than 0/1 I think. Same with
tests.
> -
>     case VT_LPSTR:
> -
>        if(!propvarIn->u.pszVal)
> -
>            return DISP_E_TYPEMISMATCH;
> -
> -
>        if(lstrcmpiA(propvarIn->u.pszVal, str_true) == 0)
> -
>        {
> -
>            *ret = 1;
> -
>            return S_OK;
> -
>        }
> -
>        if(lstrcmpiA(propvarIn->u.pszVal, str_false) == 0)
> -
>        {
> -
>            *ret = 0;
> -
>            return S_OK;
> -
>        }
> -
>        break;
> - }
> -
> - hr = PROPVAR_ConvertNumber(propvarIn, 64, FALSE, &res);
> - *ret = !!res;
> - return hr;
>
> +}
> -
>
> HRESULT WINAPI PropVariantToStringAlloc(REFPROPVARIANT propvarIn, WCHAR **ret)
>  {
>      WCHAR *res = NULL;
> diff --git a/dlls/propsys/tests/propsys.c b/dlls/propsys/tests/propsys.c
> index 776ccd0bfd..5f77f60e5f 100644
> --- a/dlls/propsys/tests/propsys.c
> +++ b/dlls/propsys/tests/propsys.c
> @@ -883,6 +883,152 @@ static void test_intconversions(void)
>      ok(llval == -7, "got wrong value %s\n", wine_dbgstr_longlong(llval));
>  }
>
> +static void test_PropVariantToBoolean(void)
> +{
> - PROPVARIANT propvar;
> - BOOL val;
> - HRESULT hr;
> - static WCHAR str_0[] = {'0',0};
> - static WCHAR str_1[] = {'1',0};
> - static WCHAR str_7[] = {'7',0};
> - static WCHAR str_true[] = {'t','r','u','e',0};
> - static WCHAR str_true_case[] = {'t','R','U','e',0};
> - static WCHAR str_false[] = {'f','a','l','s','e',0};
> - static WCHAR str_true_space[] = {'t','r','u','e',' ',0};
> - static WCHAR str_yes[] = {'y','e','s',0};
> - static CHAR str_true_ansi[] = {'t','R','U','e',0};
> - static CHAR str_false_ansi[] = {'f','A','L','S','e',0};
> - static CHAR str_1_ansi[] = {'1',0};
> -
> - /* test booleans */
> -
> - propvar.vt = VT_BOOL;
> - propvar.u.boolVal = 0;
> - hr = PropVariantToBoolean(&propvar, &val);
> - ok(hr == S_OK, "hr=%x\n", hr);
> - ok(val == 0, "got wrong value %d\n", val);

Better name for 0 in this context is VARIANT_FALSE.
> -
> - propvar.vt = VT_LPWSTR;
> - propvar.u.pwszVal = 0;

NULL.

[PATCH] propsys: Add PropVariantToBoolean and tests

[Fabian Maurer]

> Is it correct to crash on NULL argument?

I just tested it, and it indeed crashes on NULL.
How can I write a test for that?

I'll fix the other issues.

Regards,
Fabian Maurer

[PATCH] propsys: Add PropVariantToBoolean and tests

[Nikolay Sivov]

On 18.09.2017 22:22, Fabian Maurer wrote:
> > Is it correct to crash on NULL argument?
>
> I just tested it, and it indeed crashes on NULL.
> How can I write a test for that?

No need to test it. Checking manually as you did is enough.
> I'll fix the other issues.
>
> Regards,
> Fabian Maurer