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

[Nikolay Sivov]

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.
> /* 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.
> -static ULONG WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_Release (LPDIRECTMUSICLOADER8 iface) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> -
>
> +static ULONG WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_Release(LPDIRECTMUSICLOADER8 iface)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> - DWORD dwRef = InterlockedDecrement (&This->dwRef);
>  TRACE("(%p): ReleaseRef to %d\n", This, This->dwRef);
>  if (dwRef == 0) {
>
> @@ -107,8 +115,9 @@ static ULONG WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_Release (LPDIRECTM
>    return dwRef;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_GetObject (LPDIRECTMUSICLOADER8 iface, LPDMUS_OBJECTDESC pDesc, REFIID riid, LPVOID* ppv) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_GetObject(LPDIRECTMUSICLOADER8 iface, LPDMUS_OBJECTDESC pDesc, REFIID riid, LPVOID* ppv)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> HRESULT result = S_OK;
> HRESULT ret = S_OK; /* used at the end of function, to determine whether everything went OK */
>
> @@ -361,8 +370,9 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_GetObject (LPDIR
>    	return result;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetObject (LPDIRECTMUSICLOADER8 iface, LPDMUS_OBJECTDESC pDesc) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetObject(LPDIRECTMUSICLOADER8 iface, LPDMUS_OBJECTDESC pDesc)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> LPSTREAM pStream;
> LPDIRECTMUSICOBJECT pObject;
> DMUS_OBJECTDESC Desc;
>
> @@ -483,9 +493,10 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetObject (LPDIR
>    return S_OK;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetSearchDirectory (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, WCHAR* pwzPath, BOOL fClear) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetSearchDirectory(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, WCHAR* pwzPath, BOOL fClear)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> WCHAR wszCurrentPath[MAX_PATH];
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %s, %s, %d)\n", This, debugstr_dmguid(rguidClass), debugstr_w(pwzPath), fClear);
> FIXME(": fClear ignored\n");
> DMUSIC_GetLoaderSettings (iface, rguidClass, wszCurrentPath, NULL);
>
> @@ -496,14 +507,15 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_SetSearchDirecto
>    return DMUSIC_SetLoaderSettings (iface, rguidClass, pwzPath, NULL);
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ScanDirectory (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, WCHAR* pwzFileExtension, WCHAR* pwzScanFileName) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ScanDirectory(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, WCHAR* pwzFileExtension, WCHAR* pwzScanFileName)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> static const WCHAR wszAny[] = {'*',0};
> WIN32_FIND_DATAW FileData;
> HANDLE hSearch;
> WCHAR wszSearchString[MAX_PATH];
> WCHAR *p;
> HRESULT result;
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %s, %p, %p)\n", This, debugstr_dmguid(rguidClass), pwzFileExtension, pwzScanFileName);
> if (IsEqualGUID (rguidClass, &GUID_DirectMusicAllTypes) || !DMUSIC_IsValidLoadableClass(rguidClass)) {
> ERR(": rguidClass invalid CLSID\n");
>
> @@ -550,13 +562,14 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ScanDirectory (L
>    } while (1);
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_CacheObject (LPDIRECTMUSICLOADER8 iface, IDirectMusicObject* pObject) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_CacheObject(LPDIRECTMUSICLOADER8 iface, IDirectMusicObject* pObject)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> DMUS_OBJECTDESC Desc;
> HRESULT result = DMUS_E_LOADER_OBJECTNOTFOUND;
> struct list *pEntry;
> LPWINE_LOADER_ENTRY  pObjectEntry = NULL;
> -
>
> ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %p)\n", This, pObject);
>
> /* get descriptor */
>
> @@ -630,13 +643,14 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_CacheObject (LPD
>    return result;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObject (LPDIRECTMUSICLOADER8 iface, IDirectMusicObject* pObject) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObject(LPDIRECTMUSICLOADER8 iface, IDirectMusicObject* pObject)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> DMUS_OBJECTDESC Desc;
> struct list *pEntry;
> LPWINE_LOADER_ENTRY pObjectEntry = NULL;
> HRESULT result = S_FALSE;
> -
>
> ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %p)\n", This, pObject);
>
> if(!pObject) return E_POINTER;
>
> @@ -695,10 +709,11 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObject (L
>    return result;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ClearCache (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ClearCache(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> struct list *pEntry;
> LPWINE_LOADER_ENTRY pObjectEntry;
> -
>
> ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %s)\n", This, debugstr_dmguid(rguidClass));
>
> LIST_FOR_EACH (pEntry, This->pObjects) {
>
> @@ -714,8 +729,9 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ClearCache (LPDI
>    return S_OK;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnableCache (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, BOOL fEnable) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnableCache(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, BOOL fEnable)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> BOOL bCurrent;
> TRACE("(%p, %s, %d)\n", This, debugstr_dmguid(rguidClass), fEnable);
> DMUSIC_GetLoaderSettings (iface, rguidClass, NULL, &bCurrent);
>
> @@ -725,11 +741,12 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnableCache (LPD
>    	return DMUSIC_SetLoaderSettings (iface, rguidClass, NULL, &fEnable);
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnumObject (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, DWORD dwIndex, LPDMUS_OBJECTDESC pDesc) {
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnumObject(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClass, DWORD dwIndex, LPDMUS_OBJECTDESC pDesc)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> DWORD dwCount = 0;
> struct list *pEntry;
> LPWINE_LOADER_ENTRY pObjectEntry;
> -
>
> ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> TRACE("(%p, %s, %d, %p)\n", This, debugstr_dmguid(rguidClass), dwIndex, pDesc);
>
> DM_STRUCT_INIT(pDesc);
>
> @@ -755,13 +772,15 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_EnumObject (LPDI
>    return S_FALSE;
>   }
>
> -static void WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_CollectGarbage (LPDIRECTMUSICLOADER8 iface) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static void WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_CollectGarbage(LPDIRECTMUSICLOADER8 iface)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> FIXME("(%p): stub\n", This);
> }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObjectByUnknown (LPDIRECTMUSICLOADER8 iface, IUnknown* pObject) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObjectByUnknown(LPDIRECTMUSICLOADER8 iface, IUnknown* pObject)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> HRESULT result;
> LPDIRECTMUSICOBJECT pObjectInterface;
>
> @@ -781,8 +800,9 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_ReleaseObjectByU
>    return result;
>   }
>
> -static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_LoadObjectFromFile (LPDIRECTMUSICLOADER8 iface, REFGUID rguidClassID, REFIID iidInterfaceID, WCHAR* pwzFilePath, void** ppObject) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT WINAPI IDirectMusicLoaderImpl_IDirectMusicLoader_LoadObjectFromFile(LPDIRECTMUSICLOADER8 iface, REFGUID rguidClassID, REFIID iidInterfaceID, WCHAR* pwzFilePath, void** ppObject)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> DMUS_OBJECTDESC ObjDesc;
> WCHAR wszLoaderSearchPath[MAX_PATH];
>
> @@ -859,7 +879,7 @@ HRESULT WINAPI DMUSIC_CreateDirectMusicLoaderImpl (LPCGUID lpcGUID, LPVOID *ppob
>    	*ppobj = NULL;
>    	return E_OUTOFMEMORY;
>    }
> - obj->LoaderVtbl = &DirectMusicLoader_Loader_Vtbl;
> - obj->IDirectMusicLoader8_iface.lpVtbl = &DirectMusicLoader_Loader_Vtbl;
> obj->dwRef = 0; /* will be inited with QueryInterface */
> /* init cache/alias list */
> obj->pObjects = HeapAlloc (GetProcessHeap (), HEAP_ZERO_MEMORY, sizeof(struct list));
>
> @@ -867,7 +887,7 @@ HRESULT WINAPI DMUSIC_CreateDirectMusicLoaderImpl (LPCGUID lpcGUID, LPVOID *ppob
>    /* init settings */
>    obj->pClassSettings = HeapAlloc (GetProcessHeap (), HEAP_ZERO_MEMORY, sizeof(struct list));
>    list_init (obj->pClassSettings);
> - DMUSIC_InitLoaderSettings ((LPDIRECTMUSICLOADER8)obj);
> -
>
> DMUSIC_InitLoaderSettings(&obj->IDirectMusicLoader8_iface);
>
> /* set default DLS collection (via SetObject... so that loading via DMUS_OBJ_OBJECT is possible) */
> DM_STRUCT_INIT(&Desc);
>
> @@ -875,7 +895,7 @@ HRESULT WINAPI DMUSIC_CreateDirectMusicLoaderImpl (LPCGUID lpcGUID, LPVOID *ppob
>    Desc.guidClass = CLSID_DirectMusicCollection;
>    Desc.guidObject = GUID_DefaultGMCollection;
>    DMUSIC_GetDefaultGMPath (Desc.wszFileName);
> - IDirectMusicLoader_SetObject ((LPDIRECTMUSICLOADER8)obj, &Desc);
> - IDirectMusicLoader_SetObject(&obj->IDirectMusicLoader8_iface, &Desc);
> /* and now the workaroundTM for "invalid" default DLS; basically,
>  my tests showed that if GUID chunk is present in default DLS
>  collection, loader treats it as "invalid" and returns
>
> @@ -889,12 +909,13 @@ HRESULT WINAPI DMUSIC_CreateDirectMusicLoaderImpl (LPCGUID lpcGUID, LPVOID *ppob
>       lock_module();
>
> - return IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface ((LPDIRECTMUSICLOADER8)obj, lpcGUID, ppobj);
> -
>
> return IDirectMusicLoader_QueryInterface(&obj->IDirectMusicLoader8_iface, lpcGUID, ppobj);
> }
>
> /* help function for retrieval of search path and caching option for certain class */
>
> -static HRESULT DMUSIC_GetLoaderSettings (LPDIRECTMUSICLOADER8 iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT DMUSIC_GetLoaderSettings(LPDIRECTMUSICLOADER8 iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> struct list *pEntry;
> TRACE(": (%p, %s, %p, %p)\n", This, debugstr_dmguid(pClassID), wszSearchPath, pbCache);
>
> @@ -912,8 +933,9 @@ static HRESULT DMUSIC_GetLoaderSettings (LPDIRECTMUSICLOADER8 iface, REFGUID pCl
>   }
>
> /* help function for setting search path and caching option for certain class */
> -static HRESULT DMUSIC_SetLoaderSettings (LPDIRECTMUSICLOADER8 iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
>
> +static HRESULT DMUSIC_SetLoaderSettings(LPDIRECTMUSICLOADER8 iface, REFGUID pClassID, WCHAR* wszSearchPath, LPBOOL pbCache)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> struct list *pEntry;
> HRESULT result = S_FALSE; /* in case pClassID != GUID_DirectMusicAllTypes and not a valid CLSID */
> TRACE(": (%p, %s, %p, %p)\n", This, debugstr_dmguid(pClassID), wszSearchPath, pbCache);
>
> @@ -935,9 +957,10 @@ static HRESULT DMUSIC_SetLoaderSettings (LPDIRECTMUSICLOADER8 iface, REFGUID pCl
>    return result;
>   }
>
> -static HRESULT DMUSIC_InitLoaderSettings (LPDIRECTMUSICLOADER8 iface) {
> - ICOM_THIS_MULTI(IDirectMusicLoaderImpl, LoaderVtbl, iface);
> -
>
> +static HRESULT DMUSIC_InitLoaderSettings(LPDIRECTMUSICLOADER8 iface)
> +{
> - IDirectMusicLoaderImpl *This = impl_from_IDirectMusicLoader8(iface);
> - /* hard-coded list of classes */
>  static REFCLSID classes[] = {
>   &CLSID_DirectMusicAudioPathConfig,