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,

[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.

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

[Michael Stefaniuc]

On 11/06/2012 08:51 PM, Christian Costa wrote:
> Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> > On 11/6/2012 20:47, Christian Costa wrote:
> > 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

I disagree. It is redundant, too long and thus hard to read.
I even have patches that remove some of them. I've never submitted those
as you beat me to the dm* COM cleanup.
> 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 ?

You don't need to make a distinction as you have in most of the cases
only one implementation. The stuff at the moment is highly duplicated.
Needs either to use a solution like strmbase where a base object is
included or COM aggregation. Didn't dig too deep into that as I stopped
my COM cleanup work for dm* for now.
> 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.

Sure, separate patch to remove the duplicate name is fine. Just start
with that as it makes the subsequent COM cleanup patch shorter as it
cleans up the function header (whitespace and LPJUNK).

bye
    michael

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

[Christian Costa]

Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
> On 11/06/2012 08:51 PM, Christian Costa wrote:
> > Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> > > On 11/6/2012 20:47, Christian Costa wrote:
> > > 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
>
> I disagree. It is redundant, too long and thus hard to read.
> I even have patches that remove some of them. I've never submitted those
> as you beat me to the dm* COM cleanup.

I agree too if there is only one interface. I think it is but I would 
like to be sure before submitting a patch.
I will check it and submit a patch for that.
Too long and hard to read, I don't agree. See my response to Nikolay.
> > 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 ?
>
> You don't need to make a distinction as you have in most of the cases
> only one implementation. The stuff at the moment is highly duplicated.
> Needs either to use a solution like strmbase where a base object is
> included or COM aggregation. Didn't dig too deep into that as I stopped
> my COM cleanup work for dm* for now.

I agree in case thare is only one interface and only one implementation.
I don't know what you mean by "The stuff at the moment is highly 
duplicated".
strmbase mostly exists because there is such a library in SDK.
> > 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.
>
> Sure, separate patch to remove the duplicate name is fine. Just start
> with that as it makes the subsequent COM cleanup patch shorter as it
> cleans up the function header (whitespace and LPJUNK).

That's what I try to do altough I don't clearly understand what the 
problem with LPJUNK stuff.

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

[Nikolay Sivov]

On 11/7/2012 01:05, Christian Costa wrote:
> Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
> > On 11/06/2012 08:51 PM, Christian Costa wrote:
> > > Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> > > > On 11/6/2012 20:47, Christian Costa wrote:
> > > > 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
> >
> > I disagree. It is redundant, too long and thus hard to read.
> > I even have patches that remove some of them. I've never submitted those
> > as you beat me to the dm* COM cleanup.
>
> I agree too if there is only one interface. I think it is but I would 
> like to be sure before submitting a patch.
> I will check it and submit a patch for that.
> Too long and hard to read, I don't agree. See my response to Nikolay.

Your example of having two methods with the same name in same object 
could be different things:

- two separate not inherited interfaces will have IUnknown part in 
common at least, in this case it doesn't make sense
   to have traces from all of them, you use "main" interface to 
implement IUnknown part and forward to it. Having methods like
   <maininterface>_<another interface>_AddRef is strange when you can 
reduce that to <another inerface>_AddRef
- object implements interface that overrides some of methods from an 
interface it's inherited, this is a bit special and in dwrite for 
example it's done like dwritetextlayout_layout_GetFontCollection() and 
dwritetextlayout_GetFontCollection(). But that is more an exception 
because of dwrite C++ nature;

Why it's good to have short and clean names? Because that will get you 
clean traces where name doesn't eat half of line width.

For that particular case dmloader_ is enough to prefix names IMO.
> > > 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.
> >
> > Sure, separate patch to remove the duplicate name is fine. Just start
> > with that as it makes the subsequent COM cleanup patch shorter as it
> > cleans up the function header (whitespace and LPJUNK).
>
> That's what I try to do altough I don't clearly understand what the 
> problem with LPJUNK stuff.

Why would you use LPVOID, LPDWORD or LPCWSTR when it could be cleanly 
expressed without typedefs? Also naming convention of having 
type-dependent prefix like dwLen or lpcszFileName is just a noise, why 
have 5 letters prefix that supposed to express variable type, I'll check 
type anyway, and lpcszFileName turns to filename with no info loss.

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

[Christian Costa]

2012/11/7 Nikolay Sivov
> On 11/7/2012 01:05, Christian Costa wrote:
> > Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
> > > On 11/06/2012 08:51 PM, Christian Costa wrote:
> > > > Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> > > > > On 11/6/2012 20:47, Christian Costa wrote:
> > > > > 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
> > >
> > > I disagree. It is redundant, too long and thus hard to read.
> > > I even have patches that remove some of them. I've never submitted those
> > > as you beat me to the dm* COM cleanup.
> >
> > I agree too if there is only one interface. I think it is but I would
> > like to be sure before submitting a patch.
> > I will check it and submit a patch for that.
> > Too long and hard to read, I don't agree. See my response to Nikolay.
>
> Your example of having two methods with the same name in same object could
> be different things:
> - two separate not inherited interfaces will have IUnknown part in common
>
> at least, in this case it doesn't make sense
>   to have traces from all of them, you use "main" interface to implement
> IUnknown part and forward to it.

I just use an example to show that we can have methods with the same name.
This other cases too off course.

Having methods like
> <maininterface>_<another interface>_AddRef is strange when you can
> reduce that to <another inerface>_AddRef

I didn't write this code and I don't like the current name either but a
method name does not mean anything if you don't know the interface it
belongs to.
And I would put the real interface name as for the method. It's a bit like
renaming GetBufferDataPointer method to getbuf just because it's shorter.
> - object implements interface that overrides some of methods from an
>
> interface it's inherited, this is a bit special and in dwrite for example
> it's done like dwritetextlayout_layout_**GetFontCollection() and
> dwritetextlayout_**GetFontCollection(). But that is more an exception
> because of dwrite C++ nature;

The method name is GetFontCollection but what is the interface ? ILayout,
ITextLayout, ... I have to dig in the code and do some grep.
And why don't you use getfontcollection instead, or gfntcollec?
> Why it's good to have short and clean names? Because that will get you
> clean traces where name doesn't eat half of line width.

It seems "clean" is somewhat subjective. I prefer also short line in log
but this meaningfull name.
Just take quartz with object that can have up to 10 interfaces and
interfaces that can be implemented by up to 10 objects.
Put a bit of multithreading on top of it (and I don't mention the fact the
TRACE are not serialized between thread).
> For that particular case dmloader_ is enough to prefix names IMO.

Sorry I disagree but it seems it's more a matter of taste. Personally I
never get annoyed with line size
as I remember.
> > > 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.
> > >
> > > Sure, separate patch to remove the duplicate name is fine. Just start
> > > with that as it makes the subsequent COM cleanup patch shorter as it
> > > cleans up the function header (whitespace and LPJUNK).
> >
> > That's what I try to do altough I don't clearly understand what the
> > problem with LPJUNK stuff.
>
> Why would you use LPVOID, LPDWORD or LPCWSTR when it could be cleanly
> expressed without typedefs?

So apart from const problem it's more a style question. I'm ok with that.
If you have looked even more thoroughly on my patches,
you should have noticed that I started to clean this when it does not
increased patch size too much.

BTW which whitespaces ?

Also naming convention of having type-dependent prefix like dwLen or
> lpcszFileName is just a noise, why have 5 letters prefix that supposed to
> express variable type, I'll check type anyway, and lpcszFileName turns to
> filename with no info loss.
>
> I already know that. None of my patches use then and I clean them when I

can like in my second patch you didn't read.
I repeat once again. I didn't write this code.

Just for the story I recently rename hwnd to wnd and lppnt to point when
modifying ClientToScreen and I was told it was not needed.
So that would be good if reviewers agree on the same rules and put them
somewhere in the wiki as well as cleanup task (as it was done for COM
cleanup).

And please look at the whole serie before commenting and review what the
patch does and not the badness of the previous code.

Christian

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

[Michael Stefaniuc]

On 11/07/2012 02:50 PM, Christian Costa wrote:
> 2012/11/7 Nikolay Sivov <[e-mail] >
> On 11/7/2012 01:05, Christian Costa wrote:
>
>     Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
>
>         On 11/06/2012 08:51 PM, Christian Costa wrote:
>
>             Le 06/11/2012 20:26, Nikolay Sivov a écrit :
>
>                 On 11/6/2012 20:47, Christian Costa wrote:
>                 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
>
>         I disagree. It is redundant, too long and thus hard to read.
>         I even have patches that remove some of them. I've never
>         submitted those
>         as you beat me to the dm* COM cleanup.
>
>     I agree too if there is only one interface. I think it is but I
>     would like to be sure before submitting a patch.
>     I will check it and submit a patch for that.
>     Too long and hard to read, I don't agree. See my response to
>     Nikolay.
>
> Your example of having two methods with the same name in same object
> could be different things:
>
> - two separate not inherited interfaces will have IUnknown part in
> common at least, in this case it doesn't make sense
>   to have traces from all of them, you use "main" interface to
> implement IUnknown part and forward to it.
>
>
> I just use an example to show that we can have methods with the same
> name. This other cases too off course.
> Having methods like
>   <maininterface>_<another interface>_AddRef is strange when you can
> reduce that to <another inerface>_AddRef
>
>
> I didn't write this code and I don't like the current name either but a
> method name does not mean anything if you don't know the interface it
> belongs to.
> And I would put the real interface name as for the method. It's a bit
> like renaming GetBufferDataPointer method to getbuf just because it's
> shorter.

Actually that's what Nikolay said. The function name needs to be
<interface>Impl_<method> where interface/method are the official names
from the API. What is superfluous/redundant information is to prepend
all that with the object name. Which in the case of dm* is the name of
the primary interface + "Impl".
> - object implements interface that overrides some of methods from an
> interface it's inherited, this is a bit special and in dwrite for
> example it's done like dwritetextlayout_layout___GetFontCollection()
> and dwritetextlayout___GetFontCollection(). But that is more an
> exception because of dwrite C++ nature;
>
>
> The method name is GetFontCollection but what is the interface ?
> ILayout, ITextLayout, ... I have to dig in the code and do some grep.
> And why don't you use getfontcollection instead, or gfntcollec?
> Why it's good to have short and clean names? Because that will get
> you clean traces where name doesn't eat half of line width.
>
>
> It seems "clean" is somewhat subjective. I prefer also short line in log
> but this meaningfull name.
> Just take quartz with object that can have up to 10 interfaces and
> interfaces that can be implemented by up to 10 objects.
> Put a bit of multithreading on top of it (and I don't mention the fact
> the TRACE are not serialized between thread).

I the case of quartz most of the duplicated interfaces are implemented
by the same base (C++) object. Or should be implemented that way. So
while multiple COM classes support the same interface most of the time
there is one implementation only.

If not the next question to ask is if it really makes sense to have two
monster COM classes in the same .c file.

Of course Alexandre might not be convinced about a file split but in
that case to avoid functions with the same name use something short to
prefix it. I prefer using the upper letters of the COM class as a
prefix, e.g.: dsb_IUnknown_QueryInterface as opposed to
IDirectSoundBuffer8Impl_IUnknown_QueryInterface. By the time I get to
the method name I have forgotten the starting part of the function name.
Also I don't get confused by seeing two interface names in the same
function name.

bye
    michael

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

[Christian Costa]

2012/11/7 Michael Stefaniuc
> On 11/07/2012 02:50 PM, Christian Costa wrote:
> > 2012/11/7 Nikolay Sivov <[e-mail] <mailto:
>
> [e-mail]>>
> > On 11/7/2012 01:05, Christian Costa wrote:
> >
> >     Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
> >
> >         On 11/06/2012 08:51 PM, Christian Costa wrote:
> >
> >             Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> >
> >                 On 11/6/2012 20:47, Christian Costa wrote:
> >                 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
> >
> >         I disagree. It is redundant, too long and thus hard to read.
> >         I even have patches that remove some of them. I've never
> >         submitted those
> >         as you beat me to the dm* COM cleanup.
> >
> >     I agree too if there is only one interface. I think it is but I
> >     would like to be sure before submitting a patch.
> >     I will check it and submit a patch for that.
> >     Too long and hard to read, I don't agree. See my response to
> >     Nikolay.
> >
> > Your example of having two methods with the same name in same object
> > could be different things:
> >
> > - two separate not inherited interfaces will have IUnknown part in
> > common at least, in this case it doesn't make sense
> >   to have traces from all of them, you use "main" interface to
> > implement IUnknown part and forward to it.
> >
> >
> > I just use an example to show that we can have methods with the same
> > name. This other cases too off course.
> > Having methods like
> >   <maininterface>_<another interface>_AddRef is strange when you can
> > reduce that to <another inerface>_AddRef
> >
> >
> > I didn't write this code and I don't like the current name either but a
> > method name does not mean anything if you don't know the interface it
> > belongs to.
> > And I would put the real interface name as for the method. It's a bit
> > like renaming GetBufferDataPointer method to getbuf just because it's
> > shorter.
>
> Actually that's what Nikolay said. The function name needs to be
> <interface>Impl_<method> where interface/method are the official names
> from the API. What is superfluous/redundant information is to prepend
> all that with the object name. Which in the case of dm* is the name of
> the primary interface + "Impl".

So in this particular case: IDirectMusicLoaderImpl_AddRef, right ?
> > - object implements interface that overrides some of methods from an
> > interface it's inherited, this is a bit special and in dwrite for
> > example it's done like dwritetextlayout_layout___GetFontCollection()
> > and dwritetextlayout___GetFontCollection(). But that is more an
> > exception because of dwrite C++ nature;
> >
> >
> > The method name is GetFontCollection but what is the interface ?
> > ILayout, ITextLayout, ... I have to dig in the code and do some grep.
> > And why don't you use getfontcollection instead, or gfntcollec?
> > Why it's good to have short and clean names? Because that will get
> > you clean traces where name doesn't eat half of line width.
> >
> >
> > It seems "clean" is somewhat subjective. I prefer also short line in log
> > but this meaningfull name.
> > Just take quartz with object that can have up to 10 interfaces and
> > interfaces that can be implemented by up to 10 objects.
> > Put a bit of multithreading on top of it (and I don't mention the fact
> > the TRACE are not serialized between thread).
>
> I the case of quartz most of the duplicated interfaces are implemented
> by the same base (C++) object. Or should be implemented that way. So
> while multiple COM classes support the same interface most of the time
> there is one implementation only.

It's partly true. Sometimes some methods are overridden.
>
> If not the next question to ask is if it really makes sense to have two
> monster COM classes in the same .c file.

Yes of course but you will have the same function name in the debugger and
in the trace.
For example in dmusic/port.c there is 3 port objects for Synth,
MidiOut and MidiIn each one supported 3 interfaces
IDirectMusicPort, IDirectMusicPassThru
and possibly IDirectMusicPortDownload. Altough it is ok to use for
example IDirectMusicPortImpl_<Method>
for the common methods implementation but you will need to specify the
object name
for the methods that are specific to each port. Unless you keep a generic
implementation and use
a functions table as it is done sometimes in strmbase but is less C++ like.
> Of course Alexandre might not be convinced about a file split but in
> that case to avoid functions with the same name use something short to
> prefix it. I prefer using the upper letters of the COM class as a
> prefix, e.g.: dsb_IUnknown_QueryInterface as opposed to
> IDirectSoundBuffer8Impl_IUnknown_QueryInterface. By the time I get to
> the method name I have forgotten the starting part of the function name.
> Also I don't get confused by seeing two interface names in the same
> function name.
>
> Personally I would have used :

dsb_IUnknown_QueryInterface
dsb_IDirectSoundBuffer8_QueryInterface
dsb_Ixxxx_yyyy

So we have object_interface_method. This is more formal and clear and we
can remove the "Impl" suffix.
Only one simple rule and if we keep the object name short the function name
is not too long.

Christian

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

[Michael Stefaniuc]

On 11/07/2012 06:40 PM, Christian Costa wrote:
> 2012/11/7 Michael Stefaniuc <[e-mail]
> >
>     On 11/07/2012 02:50 PM, Christian Costa wrote:
>     > I didn't write this code and I don't like the current name either
>     but a
>     > method name does not mean anything if you don't know the interface it
>     > belongs to.
>     > And I would put the real interface name as for the method. It's a bit
>     > like renaming GetBufferDataPointer method to getbuf just because it's
>     > shorter.
>     Actually that's what Nikolay said. The function name needs to be
>     <interface>Impl_<method> where interface/method are the official names
>     from the API. What is superfluous/redundant information is to prepend
>     all that with the object name. Which in the case of dm* is the name of
>     the primary interface + "Impl".
>
> So in this particular case: IDirectMusicLoaderImpl_AddRef, right ?

Right.
> >     - object implements interface that overrides some of methods
> from an
> >     interface it's inherited, this is a bit special and in dwrite for
> >     example it's done like
> dwritetextlayout_layout___GetFontCollection()
> >     and dwritetextlayout___GetFontCollection(). But that is more an
> >     exception because of dwrite C++ nature;
> >
> >
> > The method name is GetFontCollection but what is the interface ?
> > ILayout, ITextLayout, ... I have to dig in the code and do some grep.
> > And why don't you use getfontcollection instead, or gfntcollec?
> >
> >
> >     Why it's good to have short and clean names? Because that will get
> >     you clean traces where name doesn't eat half of line width.
> >
> >
> > It seems "clean" is somewhat subjective. I prefer also short line
> in log
> > but this meaningfull name.
> > Just take quartz with object that can have up to 10 interfaces and
> > interfaces that can be implemented by up to 10 objects.
> > Put a bit of multithreading on top of it (and I don't mention the fact
> > the TRACE are not serialized between thread).
> I the case of quartz most of the duplicated interfaces are implemented
> by the same base (C++) object. Or should be implemented that way. So
> while multiple COM classes support the same interface most of the time
> there is one implementation only.
>
>
> It's partly true. Sometimes some methods are overridden.
> If not the next question to ask is if it really makes sense to have two
> monster COM classes in the same .c file.
>
>
> Yes of course but you will have the same function name in the debugger and
> in the trace.

I'll pass on the debugger as I'm not a debugger person.

TRACE on the other hand is simple. Sometimes there is a different debug
channel in use. Or the trace message itself can contain the info. But I
also made the experience that more often than not one has to follow the
interface/object pointer from the TRACE to be able to distinguish
between different instances of the same COM class. I mean follow it back
to the creation/initialization of the object which gives the needed info
to find the right implementation.
> For example in dmusic/port.c there is 3 port objects for Synth,
> MidiOut and MidiIn each one supported 3 interfaces
> IDirectMusicPort, IDirectMusicPassThru
> and possibly IDirectMusicPortDownload. Altough it is ok to use for
> example IDirectMusicPortImpl_<Method>
> for the common methods implementation but you will need to specify the
> object name
> for the methods that are specific to each port. Unless you keep a
> generic implementation and use

I've seen different techniques in use, it really depends on how big the
implementations differ. It might be even solvable with an if-else
    if (This->has_ds8) // if is_primary(This)
        foo;
    else
        bar;

Other times there is a different vtable with different degrees of
methods in common.
> a functions table as it is done sometimes in strmbase but is less C++ like.

You make it sound like "less C++ like" would be bad. Quite the contrary,
not being bound by the C++ object model gives you flexibility. There was
a great LWN article on the object oriented programming/design patterns
in the Linux Kernel, it is worth a read for anybody interested in object
oriented programming in C.
> Of course Alexandre might not be convinced about a file split but in
> that case to avoid functions with the same name use something short to
> prefix it. I prefer using the upper letters of the COM class as a
> prefix, e.g.: dsb_IUnknown_QueryInterface as opposed to
> IDirectSoundBuffer8Impl_IUnknown_QueryInterface. By the time I get to
> the method name I have forgotten the starting part of the function name.
> Also I don't get confused by seeing two interface names in the same
> function name.
>
> Personally I would have used :
> dsb_IUnknown_QueryInterface
> dsb_IDirectSoundBuffer8_QueryInterface
> dsb_Ixxxx_yyyy
>
> So we have object_interface_method. This is more formal and clear and we
> can remove the "Impl" suffix.
> Only one simple rule and if we keep the object name short the function
> name is not too long.

We did think about using the object name, but there is no COM object
naming standard in Wine and some of the object names could be better.
Using object_interface_method could yield stuff like
"struct_IFooBarImpl_IFooBar_QueryInterface()" which isn't a beauty.

But using just the capitalized letters from the name of the COM class as
a prefix and skipping the "Impl" would be in hindsight the better
standard. There are still 170+ COM interfaces to clean up which is a
sizable number regardless of it being just 13% of the total interface
implementations, so we could still change the standard, especially as
the existing function/method naming standard is not strictly enforced; I
didn't bother changing "offenders" if the name was reasonable.
But I'm deferring this decision to Jacek / Alexandre as they are the
drivers of the COM standardization in Wine. I don't mind too much as I
can work with both patterns.

bye
    michael

bye
    michael

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

[Henri Verbeet]

On 8 November 2012 00:22, Michael Stefaniuc  wrote:
> But using just the capitalized letters from the name of the COM class as
> a prefix and skipping the "Impl" would be in hindsight the better
> standard. There are still 170+ COM interfaces to clean up which is a
> sizable number regardless of it being just 13% of the total interface
> implementations, so we could still change the standard, especially as
> the existing function/method naming standard is not strictly enforced; I
> didn't bother changing "offenders" if the name was reasonable.
> But I'm deferring this decision to Jacek / Alexandre as they are the
> drivers of the COM standardization in Wine. I don't mind too much as I
> can work with both patterns.

I think the only reasonable naming convention is to name things after
the implementation structure. In this case that would still end up
being "IDirectMusicLoaderImpl_...", but for a slightly different
reason. Where I agree with Nikolay is that "dmloader" would be a much
nicer name than "IDirectMusicLoaderImpl" for the implementation
structure as well, in which case you would also end up with
"dmloader_..." for method implementations.

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

[Christian Costa]

2012/11/8 Henri Verbeet
> On 8 November 2012 00:22, Michael Stefaniuc  wrote:
> > But using just the capitalized letters from the name of the COM class as
> > a prefix and skipping the "Impl" would be in hindsight the better
> > standard. There are still 170+ COM interfaces to clean up which is a
> > sizable number regardless of it being just 13% of the total interface
> > implementations, so we could still change the standard, especially as
> > the existing function/method naming standard is not strictly enforced; I
> > didn't bother changing "offenders" if the name was reasonable.
> > But I'm deferring this decision to Jacek / Alexandre as they are the
> > drivers of the COM standardization in Wine. I don't mind too much as I
> > can work with both patterns.
>
> I think the only reasonable naming convention is to name things after
> the implementation structure. In this case that would still end up
> being "IDirectMusicLoaderImpl_...", but for a slightly different
> reason. Where I agree with Nikolay is that "dmloader" would be a much
> nicer name than "IDirectMusicLoaderImpl" for the implementation
> structure as well, in which case you would also end up with
> "dmloader_..." for method implementations.

dmloader_IDirectMusicLoader_Method or dmloader_Method?
I was just saying removing the interface name was not a good thing imo or
am I missing something?

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

[Henri Verbeet]

On 8 November 2012 13:13, Christian Costa  wrote:
> dmloader_IDirectMusicLoader_Method or dmloader_Method?

E.g. "dmloader_QueryInterface()".
> I was just saying removing the interface name was not a good thing imo

Yeah, I'm just disagreeing.

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

[Michael Stefaniuc]

On 11/08/2012 01:13 PM, Christian Costa wrote:
> 2012/11/8 Henri Verbeet <[e-mail] >
> On 8 November 2012 00:22, Michael Stefaniuc <[e-mail]
> <mailto:[e-mail]>> wrote:
> > But using just the capitalized letters from the name of the COM
> class as
> > a prefix and skipping the "Impl" would be in hindsight the better
> > standard. There are still 170+ COM interfaces to clean up which is a
> > sizable number regardless of it being just 13% of the total interface
> > implementations, so we could still change the standard, especially as
> > the existing function/method naming standard is not strictly
> enforced; I
> > didn't bother changing "offenders" if the name was reasonable.
> > But I'm deferring this decision to Jacek / Alexandre as they are the
> > drivers of the COM standardization in Wine. I don't mind too much as I
> > can work with both patterns.
> >
> I think the only reasonable naming convention is to name things after
> the implementation structure. In this case that would still end up
> being "IDirectMusicLoaderImpl_...", but for a slightly different
> reason. Where I agree with Nikolay is that "dmloader" would be a much
> nicer name than "IDirectMusicLoaderImpl" for the implementation
> structure as well, in which case you would also end up with
> "dmloader_..." for method implementations.
>
>
> dmloader_IDirectMusicLoader_Method or dmloader_Method?

dmloader_IDirectMusicLoader_Method
> I was just saying removing the interface name was not a good thing imo
> or am I missing something?

Right, the interface name needs to be there as it matches the COBJMACROS
name. Basically the C macro with a prefix.

bye
    michael

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

[Nikolay Sivov]

On 11/8/2012 15:41, Michael Stefaniuc wrote:
> On 11/08/2012 01:13 PM, Christian Costa wrote:
> > 2012/11/8 Henri Verbeet <[e-mail] >
> >  On 8 November 2012 00:22, Michael Stefaniuc <[e-mail]
> >  <mailto:[e-mail]>> wrote:
> >  > But using just the capitalized letters from the name of the COM
> >  class as
> >  > a prefix and skipping the "Impl" would be in hindsight the better
> >  > standard. There are still 170+ COM interfaces to clean up which is a
> >  > sizable number regardless of it being just 13% of the total interface
> >  > implementations, so we could still change the standard, especially as
> >  > the existing function/method naming standard is not strictly
> >  enforced; I
> >  > didn't bother changing "offenders" if the name was reasonable.
> >  > But I'm deferring this decision to Jacek / Alexandre as they are the
> >  > drivers of the COM standardization in Wine. I don't mind too much as I
> >  > can work with both patterns.
> >  >
> >  I think the only reasonable naming convention is to name things after
> >  the implementation structure. In this case that would still end up
> >  being "IDirectMusicLoaderImpl_...", but for a slightly different
> >  reason. Where I agree with Nikolay is that "dmloader" would be a much
> >  nicer name than "IDirectMusicLoaderImpl" for the implementation
> >  structure as well, in which case you would also end up with
> >  "dmloader_..." for method implementations.
> >
> >
> > dmloader_IDirectMusicLoader_Method or dmloader_Method?
>
> dmloader_IDirectMusicLoader_Method

I don't think it's better than it is now.
> > I was just saying removing the interface name was not a good thing imo
> > or am I missing something?
>
> Right, the interface name needs to be there as it matches the COBJMACROS
> name. Basically the C macro with a prefix.

Why? If you really want to keep interface name the better way imho is as 
it's usually done in mshtml, like HTMLDOMTextNode_*,
so here you don't need to add anything like prefix.
> bye
>    michael

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

[Michael Stefaniuc]

On 11/08/2012 02:50 PM, Nikolay Sivov wrote:
> On 11/8/2012 15:41, Michael Stefaniuc wrote:
> > On 11/08/2012 01:13 PM, Christian Costa wrote:
> > > 2012/11/8 Henri Verbeet <[e-mail] >
> > >  On 8 November 2012 00:22, Michael Stefaniuc <[e-mail]
> > >  <mailto:[e-mail]>> wrote:
> > >  > But using just the capitalized letters from the name of the COM
> > >  class as
> > >  > a prefix and skipping the "Impl" would be in hindsight the better
> > >  > standard. There are still 170+ COM interfaces to clean up
> > >
> > > which is a
> > >      > sizable number regardless of it being just 13% of the total
> > > interface
> > >      > implementations, so we could still change the standard,
> > > especially as
> > >      > the existing function/method naming standard is not strictly
> > >      enforced; I
> > >      > didn't bother changing "offenders" if the name was reasonable.
> > >      > But I'm deferring this decision to Jacek / Alexandre as they
> > > are the
> > >      > drivers of the COM standardization in Wine. I don't mind too
> > > much as I
> > >      > can work with both patterns.
> > >      >
> > >      I think the only reasonable naming convention is to name things
> > > after
> > >      the implementation structure. In this case that would still end up
> > >      being "IDirectMusicLoaderImpl_...", but for a slightly different
> > >      reason. Where I agree with Nikolay is that "dmloader" would be a
> > > much
> > >      nicer name than "IDirectMusicLoaderImpl" for the implementation
> > >      structure as well, in which case you would also end up with
> > >      "dmloader_..." for method implementations.
> > >
> > > dmloader_IDirectMusicLoader_Method or dmloader_Method?
> >
> > dmloader_IDirectMusicLoader_Method
>
> I don't think it's better than it is now.

It is better as it doesn't contain the same ICamelCaseName twice. Of
course the object should be renamed then to dmloader.
This is a workable solution for the people that insist on having the
object name in function implementing the method.
> > > I was just saying removing the interface name was not a good thing imo
> > > or am I missing something?
> >
> > Right, the interface name needs to be there as it matches the COBJMACROS
> > name. Basically the C macro with a prefix.
>
> Why? If you really want to keep interface name the better way imho is as
> it's usually done in mshtml, like HTMLDOMTextNode_*,
> so here you don't need to add anything like prefix.

The first rule of COM in Wine is:
"Do what Jacek says, not what he does!"
;)

HTMLDOMTextNode_* aka dropping the "I" is fine too as long as you do
*not* use the same prefix for other functions. E.g. I've seen that style
used for helpers.

bye
    michael

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

[Christian Costa]

> > > > I was just saying removing the interface name was not a good thing imo
> > > > or am I missing something?
> > >
> > > Right, the interface name needs to be there as it matches the COBJMACROS
> > > name. Basically the C macro with a prefix.
> >
> > Why? If you really want to keep interface name the better way imho is as
> > it's usually done in mshtml, like HTMLDOMTextNode_*,
> > so here you don't need to add anything like prefix.
>
> The first rule of COM in Wine is:
> "Do what Jacek says, not what he does!"
> ;)

Ok. I note... ;)
> HTMLDOMTextNode_* aka dropping the "I" is fine too as long as you do
> *not* use the same prefix for other functions. E.g. I've seen that style
> used for helpers.
>
> I don't like it either altough this could considered as a private method.

Note that I think such a case happen even with the "I".

We can just mention to never use interface name (with "I" or not) in
functions that don't belong to an interface.

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

[Christian Costa]

> > I was just saying removing the interface name was not a good thing imo
> > > or am I missing something?
> >
> > Right, the interface name needs to be there as it matches the COBJMACROS
> > name. Basically the C macro with a prefix.
>
> Why? If you really want to keep interface name the better way imho is as
> it's usually done in mshtml, like HTMLDOMTextNode_*,
> so here you don't need to add anything like prefix.

_I is just 2 characters and won't cost a lot but hey seems a correct
solution for me.
And when you need to specify a class name ? Are you thinking about
something like class_HTMLDOMTextNode_* ?

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

[Christian Costa]

2012/11/8 Michael Stefaniuc
> On 11/08/2012 01:13 PM, Christian Costa wrote:
> > 2012/11/8 Henri Verbeet <[e-mail] >
> > On 8 November 2012 00:22, Michael Stefaniuc <[e-mail]
> > <mailto:[e-mail]>> wrote:
> > > But using just the capitalized letters from the name of the COM
> > class as
> > > a prefix and skipping the "Impl" would be in hindsight the better
> > > standard. There are still 170+ COM interfaces to clean up which is
>
> a
> > > sizable number regardless of it being just 13% of the total
>
> interface
> > > implementations, so we could still change the standard, especially
>
> as
> > > the existing function/method naming standard is not strictly
> > enforced; I
> > > didn't bother changing "offenders" if the name was reasonable.
> > > But I'm deferring this decision to Jacek / Alexandre as they are
>
> the
> > > drivers of the COM standardization in Wine. I don't mind too much
>
> as I
> > > can work with both patterns.
> > >
> > I think the only reasonable naming convention is to name things after
> > the implementation structure. In this case that would still end up
> > being "IDirectMusicLoaderImpl_...", but for a slightly different
> > reason. Where I agree with Nikolay is that "dmloader" would be a much
> > nicer name than "IDirectMusicLoaderImpl" for the implementation
> > structure as well, in which case you would also end up with
> > "dmloader_..." for method implementations.
> >
> >
> > dmloader_IDirectMusicLoader_Method or dmloader_Method?
>
> dmloader_IDirectMusicLoader_Method

Henri said the other. It seems there is no consensus. ;)
> > I was just saying removing the interface name was not a good thing imo
> > or am I missing something?
>
> Right, the interface name needs to be there as it matches the COBJMACROS
> name. Basically the C macro with a prefix.

It is what I was thinking. Match the macros and just add a class prefix if
needed
or just _ to avoid the conflict.

Christian

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

[Michael Stefaniuc]

On 11/08/2012 03:44 PM, Christian Costa wrote:
> 2012/11/8 Michael Stefaniuc <[e-mail]
> >
> On 11/08/2012 01:13 PM, Christian Costa wrote:
> >
> >
> > 2012/11/8 Henri Verbeet <[e-mail]
> <mailto:[e-mail]> <mailto:[e-mail]
> <mailto:[e-mail]>>>
> >
> >     On 8 November 2012 00:22, Michael Stefaniuc
> <[e-mail] <mailto:[e-mail]>
> >     <mailto:[e-mail] <mailto:[e-mail]>>> wrote:
> >     > But using just the capitalized letters from the name of the COM
> >     class as
> >     > a prefix and skipping the "Impl" would be in hindsight the
> better
> >     > standard. There are still 170+ COM interfaces to clean up
> which is a
> >     > sizable number regardless of it being just 13% of the total
> interface
> >     > implementations, so we could still change the standard,
> especially as
> >     > the existing function/method naming standard is not strictly
> >     enforced; I
> >     > didn't bother changing "offenders" if the name was reasonable.
> >     > But I'm deferring this decision to Jacek / Alexandre as they
> are the
> >     > drivers of the COM standardization in Wine. I don't mind too
> much as I
> >     > can work with both patterns.
> >     >
> >     I think the only reasonable naming convention is to name
> things after
> >     the implementation structure. In this case that would still end up
> >     being "IDirectMusicLoaderImpl_...", but for a slightly different
> >     reason. Where I agree with Nikolay is that "dmloader" would be
> a much
> >     nicer name than "IDirectMusicLoaderImpl" for the implementation
> >     structure as well, in which case you would also end up with
> >     "dmloader_..." for method implementations.
> >
> >
> > dmloader_IDirectMusicLoader_Method or dmloader_Method?
> dmloader_IDirectMusicLoader_Method
>
>
> Henri said the other. It seems there is no consensus. ;)

Of course there is consensus. The consensus is:
- "It depends on the situation"
- "There are acceptable naming conventions"
- "IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface is not one
of the acceptable solutions"
:)

bye
    michael

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

[Christian Costa]

> > Henri said the other. It seems there is no consensus. ;)
>
> Of course there is consensus. The consensus is:
> - "It depends on the situation"
> - "There are acceptable naming conventions"
> - "IDirectMusicLoaderImpl_IDirectMusicLoader_QueryInterface is not one
>
> of the acceptable solutions"
> :)

Yeahhhh ! :)

I think I will use IDirectMusicLoaderImpl_Method.

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

[Christian Costa]

2012/11/8 Michael Stefaniuc
> On 11/07/2012 06:40 PM, Christian Costa wrote:
> > 2012/11/7 Michael Stefaniuc <[e-mail]
> > >
> >     On 11/07/2012 02:50 PM, Christian Costa wrote:
> >     > I didn't write this code and I don't like the current name either
> >     but a
> >     > method name does not mean anything if you don't know the interface
>
> it
> > > belongs to.
> > > And I would put the real interface name as for the method. It's a
>
> bit
> > > like renaming GetBufferDataPointer method to getbuf just because
>
> it's
> > > shorter.
> > Actually that's what Nikolay said. The function name needs to be
> > <interface>Impl_<method> where interface/method are the official
>
> names
> > from the API. What is superfluous/redundant information is to prepend
> > all that with the object name. Which in the case of dm* is the name
>
> of
> > the primary interface + "Impl".
> >
> >
> > So in this particular case: IDirectMusicLoaderImpl_AddRef, right ?
>
> Right.
> > >     - object implements interface that overrides some of methods
> > from an
> > >     interface it's inherited, this is a bit special and in dwrite
>
> for
> > >     example it's done like
> > dwritetextlayout_layout___GetFontCollection()
> > >     and dwritetextlayout___GetFontCollection(). But that is more an
> > >     exception because of dwrite C++ nature;
> > >
> > >
> > > The method name is GetFontCollection but what is the interface ?
> > > ILayout, ITextLayout, ... I have to dig in the code and do some
>
> grep.
> > > And why don't you use getfontcollection instead, or gfntcollec?
> > >
> > >
> > >     Why it's good to have short and clean names? Because that will
>
> get
> > >     you clean traces where name doesn't eat half of line width.
> > >
> > >
> > > It seems "clean" is somewhat subjective. I prefer also short line
> > in log
> > > but this meaningfull name.
> > > Just take quartz with object that can have up to 10 interfaces and
> > > interfaces that can be implemented by up to 10 objects.
> > > Put a bit of multithreading on top of it (and I don't mention the
>
> fact
> > > the TRACE are not serialized between thread).
> > I the case of quartz most of the duplicated interfaces are
>
> implemented
> > by the same base (C++) object. Or should be implemented that way. So
> > while multiple COM classes support the same interface most of the
>
> time
> > there is one implementation only.
> >
> >
> > It's partly true. Sometimes some methods are overridden.
> > If not the next question to ask is if it really makes sense to have
>
> two
> > monster COM classes in the same .c file.
> >
> >
> > Yes of course but you will have the same function name in the debugger
>
> and
> > in the trace.
>
> I'll pass on the debugger as I'm not a debugger person.
>
> TRACE on the other hand is simple. Sometimes there is a different debug
> channel in use. Or the trace message itself can contain the info. But I
> also made the experience that more often than not one has to follow the
> interface/object pointer from the TRACE to be able to distinguish
> between different instances of the same COM class. I mean follow it back
> to the creation/initialization of the object which gives the needed info
> to find the right implementation.

Relying on that just because we use functions with the same name is awkward
imo.
> > For example in dmusic/port.c there is 3 port objects for Synth,
> > MidiOut and MidiIn each one supported 3 interfaces
> > IDirectMusicPort, IDirectMusicPassThru
> > and possibly IDirectMusicPortDownload. Altough it is ok to use for
> > example IDirectMusicPortImpl_<Method>
> > for the common methods implementation but you will need to specify the
> > object name
> > for the methods that are specific to each port. Unless you keep a
> > generic implementation and use
>
> I've seen different techniques in use, it really depends on how big the
> implementations differ. It might be even solvable with an if-else
>     if (This->has_ds8) // if is_primary(This)
>         foo;
>     else
>         bar;
>
> Other times there is a different vtable with different degrees of
> methods in common.

I know these techniques and all are good. The best one to use depends
on the situation.
> > a functions table as it is done sometimes in strmbase but is less C++
>
> like.
> You make it sound like "less C++ like" would be bad. Quite the contrary,
> not being bound by the C++ object model gives you flexibility. There was
> a great LWN article on the object oriented programming/design patterns
> in the Linux Kernel, it is worth a read for anybody interested in object
> oriented programming in C.

Well no. Was just a comment. I'm not a pro C++.
That said naming conventions should not be a constraint to choose a
particular
technique over another one.
> > Of course Alexandre might not be convinced about a file split but in
> > that case to avoid functions with the same name use something short
>
> to
> > prefix it. I prefer using the upper letters of the COM class as a
> > prefix, e.g.: dsb_IUnknown_QueryInterface as opposed to
> > IDirectSoundBuffer8Impl_IUnknown_QueryInterface. By the time I get to
> > the method name I have forgotten the starting part of the function
>
> name.
> > Also I don't get confused by seeing two interface names in the same
> > function name.
> >
> > Personally I would have used :
> > dsb_IUnknown_QueryInterface
> > dsb_IDirectSoundBuffer8_QueryInterface
> > dsb_Ixxxx_yyyy
> >
> > So we have object_interface_method. This is more formal and clear and we
> > can remove the "Impl" suffix.
> > Only one simple rule and if we keep the object name short the function
> > name is not too long.
>
> We did think about using the object name, but there is no COM object
> naming standard in Wine and some of the object names could be better.
> Using object_interface_method could yield stuff like
> "struct_IFooBarImpl_IFooBar_QueryInterface()" which isn't a beauty.

Well why struct_I*Impl ?
It seems there is a confusion between class and interface.
For example CLSID_DirectMusicLoader is the class and IDirectMusicLoader is
the interface.
Off course there are tightly tied together because CLSID_DirectMusicLoader
has only 1 interface and
IDirectMusicLoader is only implement by this class.

In that case I would just use _IDirectMusicLoader_QueryInterface() (with an
underscore prefix)
That would work also for an generic interface whose implementation is
shared by several classes.

And if we have to make a class distinction we just add the class name so we
have dmload_IDirectMusicLoader_QueryInterface.

So we have the pattern (class)_interface_method() which work in every
situation and enable homogeneity.

This concept of primary interface is a bit odd imo. Saying that IBaseFilter
is the primary interface of the video renderer does not
make sense.
>

LPJUNK for COM Interfaces (Was: Re: [PATCH 1/3] dmloader: COM cleanup of IDirectMusicLoader object.)

[Michael Stefaniuc]

On 11/06/2012 11:05 PM, Christian Costa wrote:
> > Sure, separate patch to remove the duplicate name is fine. Just start
> > with that as it makes the subsequent COM cleanup patch shorter as it
> > cleans up the function header (whitespace and LPJUNK).
>
> That's what I try to do altough I don't clearly understand what the
> problem with LPJUNK stuff.

For the "normal" LPJUNK there is no problem. Except the programmers that
use it. Then you get beautiful code like:
include/rpcasync.h:RPCRTAPI RPC_STATUS RPC_ENTRY
RpcGetAuthorizationContextForClient(RPC_BINDING_HANDLE,BOOL,LPVOID,PLARGE_INTEGER,LUID,DWORD,PVOID,PVOID*);

LPVOID, PVOID, PVOID*
In the same line.

If they would have used LPVOID, LPVOID, LPLPVOID I could have at least
appreciate their effort to stay consistent. Of course there is no
LPLPVOID...

So for the normal LPJUNK inconsistency is the main issue and there is
code out there that uses every possible permutations of saying void*. In
the same function. Adding "const" into the mix makes it more "interesting".
But overall it is easy to decode: you tread a postfix "*" for a prefix "LP":
LPVOID => VOID*
No surprises there even if you count in the two notable exceptions:
LPSTR ==> char *
LPWSTR ==> WCHAR *

The LPJUNK for COM interface types has a far bigger problem, it looses
information:
LPDIRECTSOUNDBUFFER ==> DIRECTSOUNDBUFFER* ??

Add some LPDSCBUFFERDESC and similar to the same line of code and you
lost track. I'm singling out dsound here but ddraw/d3d with their
monster functions with 10+ parameters was way worse to figure out.
Especially when the implementation was LPJUNK casting those parameters
to pass them to other functions: Is there a object <==> interface cast
hidden there? Or maybe a cast from one interface to the other where it
should instead use QueryInterface?

bye
    michael