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.