Re: ole32: Simplify a function by removing no-op code (PVS-Studio).

[Sebastian Lackner]

On 19.11.2014 09:26, Michael Stefaniuc wrote:
> MSHLFLAGS_NORMAL is 0.
> ---
>  dlls/ole32/stubmanager.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/dlls/ole32/stubmanager.c b/dlls/ole32/stubmanager.c
> index 96d710d..1aca924 100644
> --- a/dlls/ole32/stubmanager.c
> +++ b/dlls/ole32/stubmanager.c
> @@ -536,7 +536,6 @@ HRESULT ipid_get_dispatch_params(const IPID *ipid, APARTMENT **stub_apt,
>  /* returns TRUE if it is possible to unmarshal, FALSE otherwise. */
>  BOOL stub_manager_notify_unmarshal(struct stub_manager *m, const IPID *ipid)
>  {
> -    BOOL ret = TRUE;
>      struct ifstub *ifstub;
>
> if (!(ifstub = stub_manager_ipid_to_ifstub(m, ipid)))
> @@ -545,23 +544,7 @@ BOOL stub_manager_notify_unmarshal(struct stub_manager *m, const IPID *ipid)
>          return FALSE;
>      }
>
> -    EnterCriticalSection(&m->lock);
> -
> -    /* track normal marshals so we can enforce rules whilst in-process */
> -    if (ifstub->flags & MSHLFLAGS_NORMAL)
> -    {
> -        if (m->norm_refs)
> -            m->norm_refs--;
> -        else
> -        {
> -            ERR("attempted invalid normal unmarshal, norm_refs is zero\n");
> -            ret = FALSE;
> -        }
> -    }
> -
> -    LeaveCriticalSection(&m->lock);
> -
> -    return ret;
> +    return TRUE;
>  }
>
> /* handles refcounting for CoReleaseMarshalData */

Although logically correct, I am not sure if this is what it should look like.

In ole32/marshal for example there is also:

--- snip ---
    if (flags & MSHLFLAGS_TABLEWEAK)
        TRACE(" MSHLFLAGS_TABLEWEAK");
    if (!(flags & (MSHLFLAGS_TABLESTRONG|MSHLFLAGS_TABLEWEAK)))
        TRACE(" MSHLFLAGS_NORMAL");
    if (flags & MSHLFLAGS_NOPING)
        TRACE(" MSHLFLAGS_NOPING");
--- snip ---

This could probably mean that fixing the if condition is a more correct solution, but I don't know that part well enough.

ole32: Simplify a function by removing no-op code (PVS-Studio).

[Michael Stefaniuc]

On 11/19/2014 09:51 AM, Sebastian Lackner wrote:
> On 19.11.2014 09:26, Michael Stefaniuc wrote:
> > MSHLFLAGS_NORMAL is 0.
> > ---
> >   dlls/ole32/stubmanager.c | 19 +------------------
> >   1 file changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/dlls/ole32/stubmanager.c b/dlls/ole32/stubmanager.c
> > index 96d710d..1aca924 100644
> > --- a/dlls/ole32/stubmanager.c
> > +++ b/dlls/ole32/stubmanager.c
> > @@ -536,7 +536,6 @@ HRESULT ipid_get_dispatch_params(const IPID *ipid, APARTMENT **stub_apt,
> Although logically correct, I am not sure if this is what it should look like.
>
> In ole32/marshal for example there is also:
>
> --- snip ---
>      if (flags & MSHLFLAGS_TABLEWEAK)
>          TRACE(" MSHLFLAGS_TABLEWEAK");
>      if (!(flags & (MSHLFLAGS_TABLESTRONG|MSHLFLAGS_TABLEWEAK)))
>          TRACE(" MSHLFLAGS_NORMAL");
>      if (flags & MSHLFLAGS_NOPING)
>          TRACE(" MSHLFLAGS_NOPING");
> --- snip ---
>
> This could probably mean that fixing the if condition is a more correct solution, but I don't know that part well enough.

Maybe, maybe not. The code is 9-10 years old, if it wasn't a problem 
until now then going with the "obviously correct" logical transformation 
is the safer bet to not get the patch into Pending.

bye
	michael