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.

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

[Nikolay Sivov]

On 11/25/2013 11:14, Dmitry Timoshkov wrote:
> 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.

It's not changed, helper is a new addition. I think helper purpose is 
clear from its name,
argument names also help with that.
> Also
> moving the parameter checks from the callers to helper doesn't look
> justified IMO.

This helper is about to be used in more methods with similar purpose to 
return a string.