On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c index 879d8b0..42d722c 100644 --- a/dlls/d3drm/d3drm.c +++ b/dlls/d3drm/d3drm.c @@ -87,10 +87,11 @@ static HRESULT WINAPI IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface) { IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
- TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
- return InterlockedIncrement(&This->ref);
return ref; }
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface) IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface); ULONG ref = InterlockedDecrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref) HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c index 559128e..6f9802a 100644 --- a/dlls/d3drm/device.c +++ b/dlls/d3drm/device.c @@ -93,10 +93,11 @@ static HRESULT WINAPI IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2* static ULONG WINAPI IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface) { IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p)\n", This);
- TRACE("(%p): AddRef from %d\n", This, ref - 1);
- return InterlockedIncrement(&This->ref);
- return ref; }
I personally don't see a reason to trace actual reference on release and previous one on AddRef. I usually trace new value in both calls so you don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name is visible in traces already.
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c index 879d8b0..42d722c 100644 --- a/dlls/d3drm/d3drm.c +++ b/dlls/d3drm/d3drm.c @@ -87,10 +87,11 @@ static HRESULT WINAPI IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface) { IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
- TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
- return InterlockedIncrement(&This->ref);
return ref; }
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface) IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface); ULONG ref = InterlockedDecrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref) HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c index 559128e..6f9802a 100644 --- a/dlls/d3drm/device.c +++ b/dlls/d3drm/device.c @@ -93,10 +93,11 @@ static HRESULT WINAPI IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2* static ULONG WINAPI IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface) { IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p)\n", This);
- TRACE("(%p): AddRef from %d\n", This, ref - 1);
- return InterlockedIncrement(&This->ref);
- return ref; }
I personally don't see a reason to trace actual reference on release and previous one on AddRef. I usually trace new value in both calls so you don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name is visible in traces already.
Indeed that does not make sense but I've a always seen that. In addition I took the message from another place. I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I will stick to it. That said that would be good to have some recommendations so people can use follow them.
On 3/9/2012 14:46, Christian Costa wrote:
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c index 879d8b0..42d722c 100644 --- a/dlls/d3drm/d3drm.c +++ b/dlls/d3drm/d3drm.c @@ -87,10 +87,11 @@ static HRESULT WINAPI IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface) { IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
- TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
- return InterlockedIncrement(&This->ref);
return ref; }
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface) IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface); ULONG ref = InterlockedDecrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref) HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c index 559128e..6f9802a 100644 --- a/dlls/d3drm/device.c +++ b/dlls/d3drm/device.c @@ -93,10 +93,11 @@ static HRESULT WINAPI IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2* static ULONG WINAPI IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface) { IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p)\n", This);
- TRACE("(%p): AddRef from %d\n", This, ref - 1);
- return InterlockedIncrement(&This->ref);
- return ref; }
I personally don't see a reason to trace actual reference on release and previous one on AddRef. I usually trace new value in both calls so you don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name is visible in traces already.
Indeed that does not make sense but I've a always seen that. In addition I took the message from another place. I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I will stick to it.
This is too verbose too, format like (%p)->(%d) is enough. You don't usually look at refcount traces while debugging, but when you see it traced value should be consistent so you don't need to think is it 'new ref' or 'old ref'.
That said that would be good to have some recommendations so people can use follow them.
True.
Le 09/03/2012 13:49, Nikolay Sivov a écrit :
On 3/9/2012 14:46, Christian Costa wrote:
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c index 879d8b0..42d722c 100644 --- a/dlls/d3drm/d3drm.c +++ b/dlls/d3drm/d3drm.c @@ -87,10 +87,11 @@ static HRESULT WINAPI IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface) { IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
- TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
- return InterlockedIncrement(&This->ref);
return ref; }
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface) IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface); ULONG ref = InterlockedDecrement(&This->ref);
- TRACE("(%p/%p)\n", iface, This);
TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref) HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c index 559128e..6f9802a 100644 --- a/dlls/d3drm/device.c +++ b/dlls/d3drm/device.c @@ -93,10 +93,11 @@ static HRESULT WINAPI IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2* static ULONG WINAPI IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface) { IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
- ULONG ref = InterlockedIncrement(&This->ref);
- TRACE("(%p)\n", This);
- TRACE("(%p): AddRef from %d\n", This, ref - 1);
- return InterlockedIncrement(&This->ref);
- return ref; }
I personally don't see a reason to trace actual reference on release and previous one on AddRef. I usually trace new value in both calls so you don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name is visible in traces already.
Indeed that does not make sense but I've a always seen that. In addition I took the message from another place. I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I will stick to it.
This is too verbose too, format like (%p)->(%d) is enough. You don't usually look at refcount traces while debugging, but when you see it traced value should be consistent so you don't need to think is it 'new ref' or 'old ref'.
->(...) are for arguments and I would keep 'new' to avoid confusion. Maybe I could remove 'ref' but hey I don't think it's so verbose, it take one line anyway. I never have problem with refcount traces. This is the first thing I look at when I debug a crash. Using a dedicated channel for refcount would be better to reduce verbosity IMO.
-
Christian Costa -
Nikolay Sivov