Re: [PATCH 2/5] qmgr: Implement IBackgroundCopyJob::GetDescription()

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> +static HRESULT return_strval(const WCHAR *str, WCHAR **ret)
> +{
> - int len;
> -
> - if (!ret) return E_INVALIDARG;
> -
> - len = strlenW(str);
> - *ret = CoTaskMemAlloc((len+1)*sizeof(WCHAR));
> - if (!*ret) return E_OUTOFMEMORY;
> -
> - if (len)
> -
>    strcpyW(*ret, str);
> - else
> -
>    **ret = 0;
> - return S_OK;
>
> +}
> -
>
> static inline BackgroundCopyJobImpl *impl_from_IBackgroundCopyJob2(IBackgroundCopyJob2 *iface)
>  {
>      return CONTAINING_RECORD(iface, BackgroundCopyJobImpl, IBackgroundCopyJob2_iface);
> @@ -313,17 +330,10 @@ static HRESULT WINAPI BITS_IBackgroundCopyJob_GetDisplayName(
>      LPWSTR *pVal)
>  {
>      BackgroundCopyJobImpl *This = impl_from_IBackgroundCopyJob2(iface);
> -
>
> int n;
> -
>
> if (!pVal)
> -
>    return E_INVALIDARG;
> - TRACE("(%p)->(%p)\n", This, pVal);
> - n = (strlenW(This->displayName) + 1) * sizeof **pVal;
> - *pVal = CoTaskMemAlloc(n);
> - if (*pVal == NULL)
> -
>    return E_OUTOFMEMORY;
> - memcpy(*pVal, This->displayName, n);
> - return S_OK;
> - return return_strval(This->displayName, pVal);
>
> }

What is the reason of changing pVal type from LPWSTR* to WCHAR** in
the helper? Leaving would LPWSTR* make it more clear IMO what the helper
is supposed to do and would simplify dereferencing logic a bit. Also
moving the parameter checks from the callers to helper doesn't look
justified IMO.