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

[Dmitry Timoshkov]

Nikolay Sivov <nsivov(a)codeweavers.com> 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.

-- 
Dmitry.

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

[Nikolay Sivov]

On 11/25/2013 11:14, Dmitry Timoshkov wrote:
> Nikolay Sivov <nsivov(a)codeweavers.com> 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.