Re: [PATCH 2/6] gameux: initial implementation of IGameExplorer::AddGame

[Nikolay Sivov]

On 9/3/2010 13:25, Mariusz Pluciński wrote:
> ---
>  dlls/gameux/Makefile.in          |    2 +-
>  dlls/gameux/gameexplorer.c       |  254 
> +++++++++++++++++++++++++++++++++++++-
>  dlls/gameux/gameux_private.h     |   63 ++++++++++
>  dlls/gameux/tests/gameexplorer.c |   18 ++--
>  4 files changed, 322 insertions(+), 15 deletions(-)
> +/*******************************************************************************
> + * _initGameData
> + *
> + * Internal helper function. Description available in gameux_private.h file
> + */
> +void _initGameData(struct GAME_DATA *GameData)
> +{
> +    GameData->sGDFBinaryPath = NULL;
> +    GameData->sGameInstallDirectory = NULL;
> +}

Do you really need another helper for that? Please use better naming 
like GAMEUX_* for example.
> +void _uninitGameData(struct GAME_DATA *GameData)
> +{
> +    CoTaskMemFree(GameData->sGDFBinaryPath);
> +    CoTaskMemFree(GameData->sGameInstallDirectory);
> +}
> Any specific reason you need CoTaskMem* here and not a process heap?
> +    WCHAR sInstanceId[40];
> +    WCHAR sRegistryPath[8192];
> Where does this come from? Sizes I mean.

[PATCH 2/6] gameux: initial implementation of IGameExplorer::AddGame

[Mariusz Pluciński]

W dniu 03.09.2010 13:01, Nikolay Sivov pisze:
> On 9/3/2010 13:25, Mariusz Pluciński wrote:
> > ---
> > dlls/gameux/Makefile.in | 2 +-
> > dlls/gameux/gameexplorer.c | 254 +++++++++++++++++++++++++++++++++++++-
> > dlls/gameux/gameux_private.h | 63 ++++++++++
> > dlls/gameux/tests/gameexplorer.c | 18 ++--
> > 4 files changed, 322 insertions(+), 15 deletions(-)
> > +/*******************************************************************************
> >
> > + * _initGameData
> > + *
> > + * Internal helper function. Description available in
> > gameux_private.h file
> > + */
> > +void _initGameData(struct GAME_DATA *GameData)
> > +{
> > + GameData->sGDFBinaryPath = NULL;
> > + GameData->sGameInstallDirectory = NULL;
> > +}
>
> Do you really need another helper for that? Please use better naming
> like GAMEUX_* for example.
> I need this values to be initialized with NULL, to be properly freed in 
> my _uninit function. So, if adding helper isn't very well solution, 
> should I use ZeroMemory() macro, or memset() call directly?
> > +void _uninitGameData(struct GAME_DATA *GameData)
> > +{
> > + CoTaskMemFree(GameData->sGDFBinaryPath);
> > + CoTaskMemFree(GameData->sGameInstallDirectory);
> > +}
> > Any specific reason you need CoTaskMem* here and not a process heap?
> > I decided to use it, because I did not want to mix two methods of memory 
> > management in my code (CoTaskMemFree is required in some COM related 
> > areas, but mainly in code which I did not send yet). Is there any 
> > disadvantage of using it?
> > + WCHAR sInstanceId[40];
> > + WCHAR sRegistryPath[8192];
> > Where does this come from? Sizes I mean.
> > I did a mistake. sInstanceId needs only 39 characters to store GUID and 
> > zero byte for termination. Thanks.

In defining size of sRegistryPath, I followed suggestions from MSDN:
http://msdn.microsoft.com/en-us/library/ms724872%28v=VS.85%29.aspx
by multiplying 32*256 - maximum number of registry tree levels in single 
API call and maximum number of characters in key name with following '\' 
character added. I must assume, that it was a problem for me to decide 
which value should I use. Is there any better way to do this?

[PATCH 2/6] gameux: initial implementation of IGameExplorer::AddGame

[Mariusz Pluciński]

W dniu 07.09.2010 11:52, Mariusz Pluciński pisze:
> W dniu 03.09.2010 13:01, Nikolay Sivov pisze:
> > On 9/3/2010 13:25, Mariusz Pluciński wrote:
> > > + WCHAR sInstanceId[40];
> > > + WCHAR sRegistryPath[8192];
> > > Where does this come from? Sizes I mean.
> > > I did a mistake. sInstanceId needs only 39 characters to store GUID and
> > > zero byte for termination. Thanks.
>
> In defining size of sRegistryPath, I followed suggestions from MSDN:
> http://msdn.microsoft.com/en-us/library/ms724872%28v=VS.85%29.aspx
> by multiplying 32*256 - maximum number of registry tree levels in single
> API call and maximum number of characters in key name with following '\'
> character added. I must assume, that it was a problem for me to decide
> which value should I use. Is there any better way to do this?
> Oh, and I forgot to add: my function _buildGameRegistryPath is copied 
> directly from my previous commit, which was accepted:
> http://source.winehq.org/git/wine.git/?a=commitdiff;h=6cb7658377ac513e8acacd...