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.