Re: [PATCH] amstream: Display new ref in AddRef and Release.

[Nikolay Sivov]

On 10/12/2012 00:46, Christian Costa wrote:
> dlls/amstream/mediastreamfilter.c |   14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/amstream/mediastreamfilter.c b/dlls/amstream/mediastreamfilter.c
> index ee9d970..0e56a68 100644
> --- a/dlls/amstream/mediastreamfilter.c
> +++ b/dlls/amstream/mediastreamfilter.c
> @@ -133,17 +133,21 @@ static HRESULT WINAPI MediaStreamFilterImpl_QueryInterface(IMediaStreamFilter *i
>
> static ULONG WINAPI MediaStreamFilterImpl_AddRef(IMediaStreamFilter *iface)
>   {
> - return BaseFilterImpl_AddRef((IBaseFilter*)iface);
> - ULONG ref = BaseFilterImpl_AddRef((IBaseFilter*)iface);
> -
> - TRACE("(%p)->(): new ref = %u\n", iface, ref);
> -
> - return ref;
> }

You should get rid of casts like that. Also I don't see a reason to 
print anything but refcount itself life (%p)->(%u).
> static ULONG WINAPI MediaStreamFilterImpl_Release(IMediaStreamFilter *iface)
>   {
>       IMediaStreamFilterImpl *This = impl_from_IMediaStreamFilter(iface);
> - ULONG refCount = BaseFilterImpl_Release((IBaseFilter*)iface);
> - ULONG ref = BaseFilterImpl_Release((IBaseFilter*)iface);
> - TRACE("(%p)->() Release from %d\n", iface, refCount + 1);
> - TRACE("(%p)->(): new ref = %u\n", iface, ref);
> - if (!refCount)
> - if (!ref)
>  {
>      int i;
>      for (i = 0; i < This->nb_streams; i++)
>
> @@ -154,7 +158,7 @@ static ULONG WINAPI MediaStreamFilterImpl_Release(IMediaStreamFilter *iface)
>           HeapFree(GetProcessHeap(), 0, This);
>       }
> - return refCount;
> -
>
> return ref;
> }
>
> /*** IPersist methods ***/

[PATCH] amstream: Display new ref in AddRef and Release.

[Christian Costa]

Le 12/10/2012 06:32, Nikolay Sivov a écrit :
> On 10/12/2012 00:46, Christian Costa wrote:
> > dlls/amstream/mediastreamfilter.c |   14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/dlls/amstream/mediastreamfilter.c 
> > b/dlls/amstream/mediastreamfilter.c
> > index ee9d970..0e56a68 100644
> > --- a/dlls/amstream/mediastreamfilter.c
> > +++ b/dlls/amstream/mediastreamfilter.c
> > @@ -133,17 +133,21 @@ static HRESULT WINAPI 
> > MediaStreamFilterImpl_QueryInterface(IMediaStreamFilter *i
> >     static ULONG WINAPI 
> > MediaStreamFilterImpl_AddRef(IMediaStreamFilter *iface)
> >   {
> > - return BaseFilterImpl_AddRef((IBaseFilter*)iface);
> > - ULONG ref = BaseFilterImpl_AddRef((IBaseFilter*)iface);
> > -
> > - TRACE("(%p)->(): new ref = %u\n", iface, ref);
> > -
> > - return ref;
> > }
>
> You should get rid of casts like that. Also I don't see a reason to 
> print anything but refcount itself life (%p)->(%u).

If you mean cast like (IBaseFilter*) ok I'llfix it.

Regarding the trace it already has been discussed and parenthesis shoud 
be reserved for params and using
new ref because there is some code that print the old value so it is 
better to avoid confusion.
> > static ULONG WINAPI 
> >
> > MediaStreamFilterImpl_Release(IMediaStreamFilter *iface)
> >   {
> >       IMediaStreamFilterImpl *This = 
> > impl_from_IMediaStreamFilter(iface);
> > - ULONG refCount = BaseFilterImpl_Release((IBaseFilter*)iface);
> > - ULONG ref = BaseFilterImpl_Release((IBaseFilter*)iface);
> > - TRACE("(%p)->() Release from %d\n", iface, refCount + 1);
> > - TRACE("(%p)->(): new ref = %u\n", iface, ref);
> > - if (!refCount)
> > - if (!ref)
> >  {
> >      int i;
> >      for (i = 0; i < This->nb_streams; i++)
> >
> > @@ -154,7 +158,7 @@ static ULONG WINAPI 
> > MediaStreamFilterImpl_Release(IMediaStreamFilter *iface)
> >           HeapFree(GetProcessHeap(), 0, This);
> >       }
> > - return refCount;
> > - return ref;
> > }
> > /*** IPersist methods ***/