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