[PATCH 1/3] dmloader: COM cleanup of IDirectMusicLoader object.

[Christian Costa]

Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> On 11/6/2012 20:47, Christian Costa wrote:
>> ---
>>   dlls/dmloader/dmloader_private.h |   16 +++---
>>   dlls/dmloader/loader.c           |  107 
>> +++++++++++++++++++++++---------------
>>   2 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git a/dlls/dmloader/dmloader_private.h 
>> b/dlls/dmloader/dmloader_private.h
>> index 47a794c..913f3a6 100644
>> --- a/dlls/dmloader/dmloader_private.h
>> +++ b/dlls/dmloader/dmloader_private.h
>> @@ -91,14 +91,14 @@ typedef struct _WINE_LOADER_OPTION {
>>    * IDirectMusicLoaderImpl implementation structure
>>    */
>>   struct IDirectMusicLoaderImpl {
>> -    /* VTABLEs */
>> -    const IDirectMusicLoader8Vtbl *LoaderVtbl;
>> -    /* reference counter */
>> -    LONG dwRef;
>> -    /* simple cache (linked list) */
>> -    struct list *pObjects;
>> -    /* settings for certain object classes */
>> -    struct list *pClassSettings;
>> +    /* VTABLEs */
>> +    IDirectMusicLoader8 IDirectMusicLoader8_iface;
>> +    /* reference counter */
>> +    LONG dwRef;
>> +    /* simple cache (linked list) */
>> +    struct list *pObjects;
>> +    /* settings for certain object classes */
>> +    struct list *pClassSettings;
>>   };
> Please remove useless comments like VTABLES and reference counter and 
> strip names from prefix, e.g. dwRef->ref.
It's done in next patch.

>>     /* contained object entry */
>> diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c
>> index cfb3de9..54108c7 100644
>> --- a/dlls/dmloader/loader.c
>> +++ b/dlls/dmloader/loader.c
>> @@ -21,6 +21,11 @@
>>     WINE_DEFAULT_DEBUG_CHANNEL(dmloader);
>>   +static inline IDirectMusicLoaderImpl* 
>> impl_from_IDirectMusicLoader8(IDirectMusicLoader8 *iface)
>> +{
>> +    return CONTAINING_RECORD(iface, IDirectMusicLoaderImpl, 
>> IDirectMusicLoader8_iface);
>> +}
>> +
>>   static HRESULT DMUSIC_InitLoaderSettings (LPDIRECTMUSICLOADER8 iface);
>>   static HRESULT DMUSIC_GetLoaderSettings (LPDIRECTMUSICLOADER8 
>> iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache);
>>   static HRESULT DMUSIC_SetLoaderSettings (LPDIRECTMUSICLOADER8 
>> iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache);
>> @@ -69,8 +74,9 @@ static BOOL DMUSIC_IsValidLoadableClass (REFCLSID 
>> pClassID) {
>>    */
>>   /* IUnknown/IDirectMusicLoader(8) part: */
>>   -static HRESULT WINAPI 
>> IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface 
>> (LPDIRECTMUSICLOADER8 iface, REFIID riid, LPVOID *ppobj) {
>> -    ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>> +static HRESULT WINAPI 
>> IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface(LPDIRECTMUSICLOADER8 
>> iface, REFIID riid, LPVOID *ppobj)
>> +{
>> +    IDirectMusicLoaderImpl *This = 
>> impl_from_IDirectMusicLoader8(iface);
>>         TRACE("(%p, %s, %p)\n",This, debugstr_dmguid(riid), ppobj);
>>       if (IsEqualIID (riid, &IID_IUnknown) ||
>> @@ -85,15 +91,17 @@ static HRESULT WINAPI 
>> IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface (
>>       return E_NOINTERFACE;
>>   }
>>   -static ULONG WINAPI 
>> IDirectMusicLoaderImpl_IDirectMusicLoader_AddRef 
>> (LPDIRECTMUSICLOADER8 iface) {
>> -    ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>> +static ULONG WINAPI 
>> IDirectMusicLoaderImpl_IDirectMusicLoader_AddRef(LPDIRECTMUSICLOADER8 
>> iface)
>> +{
>> +    IDirectMusicLoaderImpl *This = 
>> impl_from_IDirectMusicLoader8(iface);
>>       TRACE("(%p): AddRef from %d\n", This, This->dwRef);
>>       return InterlockedIncrement (&This->dwRef);
>>   }
> What I also meant is these names are redundant, something like 
> directmusicloader_AddRef() is enough, of course you could use mixed 
> casing in names if you want, there's no strict guidelines for that afaik.
I like to see the interface name in the function name because the method 
alone does not mean anything.

Something like this:
<object name>Impl_<interface name>_<method name>
but in some case where there is only 1 interface for the object:
<interface name>Impl_<method name>
is better.

The interface name should be clearly visible IMO it's easier to read in 
a log file.
IDirectMusicLoaderImpl_IDirectMusicLoader_AddRef
And for example if an object has 2 interfaces X and Y that both derives 
from Z then you will have 2 methods with the same name.
How will you make the distinction ?

If dmloader object has really 1 interface, I would rename then 
IDirectMusicLoaderImpl_AddRef.
Although this is probably the case, I'm not sure yet. In all cases, I 
would leave this for another patch. One thing at once.