Re: [PATCH 1/5] gameux: Add implementation of IGameStatistics::SetCategoryTitle

[Nikolay Sivov]

On 9/26/2010 16:59, Mariusz Pluciński wrote:
> dlls/gameux/gamestatistics.c       |   50 
> ++++++++++++++++++++++++++++++++++-
>  dlls/gameux/tests/gamestatistics.c |   14 +++++-----
>  2 files changed, 55 insertions(+), 9 deletions(-)
> @@ -177,8 +201,30 @@ static HRESULT WINAPI GameStatisticsImpl_SetCategoryTitle(
>       WORD categoryIndex,
>       LPCWSTR title)
>   {
> - FIXME("stub\n");
> - return E_NOTIMPL;
> - HRESULT hr = S_OK;
> - DWORD dwLength;
> - GameStatisticsImpl *This = impl_from_IGameStatistics(iface);
> -
> - TRACE("(%p, %d, %s)\n", This, categoryIndex, debugstr_w(title));
> -
> - if(!title || categoryIndex>= MAX_CATEGORIES)
> -
>    hr = E_INVALIDARG;

Just return here and be done with it, this will save you hr 
initialization and unnecessary if (SUCCEEDED(hr)) after that.
> -
> - if(SUCCEEDED(hr))
> - {
> -
>    dwLength = lstrlenW(title);
> -
> -
>    if(dwLength>  MAX_CATEGORY_LENGTH)
> -
>    {
> -
>        hr = S_FALSE;
> -
>        dwLength = MAX_CATEGORY_LENGTH;
> -
>    }

Does it really copy with hr == S_FALSE ?
> -
> -
>    lstrcpynW(This->stats.categories[categoryIndex].sName,
> -
>              title, dwLength+1);
> - }
> -
> - return hr;
> }

[PATCH 1/5] gameux: Add implementation of IGameStatistics::SetCategoryTitle

[Mariusz Pluciński]

W dniu 26.09.2010 15:07, Nikolay Sivov pisze:
> On 9/26/2010 16:59, Mariusz Pluciński wrote:
> > dlls/gameux/gamestatistics.c | 50 ++++++++++++++++++++++++++++++++++-
> > dlls/gameux/tests/gamestatistics.c | 14 +++++-----
> > 2 files changed, 55 insertions(+), 9 deletions(-)
> > @@ -177,8 +201,30 @@ static HRESULT WINAPI
> > GameStatisticsImpl_SetCategoryTitle(
> > WORD categoryIndex,
> > LPCWSTR title)
> > {
> > - FIXME("stub\n");
> > - return E_NOTIMPL;
> > - HRESULT hr = S_OK;
> > - DWORD dwLength;
> > - GameStatisticsImpl *This = impl_from_IGameStatistics(iface);
> > -
> > - TRACE("(%p, %d, %s)\n", This, categoryIndex, debugstr_w(title));
> > -
> > - if(!title || categoryIndex>= MAX_CATEGORIES)
> > - hr = E_INVALIDARG;
>
> Just return here and be done with it, this will save you hr
> initialization and unnecessary if (SUCCEEDED(hr)) after that.

You're right, I'll fix it.
> > -
> > - if(SUCCEEDED(hr))
> > - {
> > - dwLength = lstrlenW(title);
> > -
> > - if(dwLength> MAX_CATEGORY_LENGTH)
> > - {
> > - hr = S_FALSE;
> > - dwLength = MAX_CATEGORY_LENGTH;
> > - }
>
> Does it really copy with hr == S_FALSE ?

Yes, this behavior is subject of one of tests.