Re: user32: Resend Check for NULL pData in DdeClientTransAction expectingto be passed a handle

[Dmitry Timoshkov]

"Jeff Latimer"  wrote:
> diff --git a/dlls/user32/dde_client.c b/dlls/user32/dde_client.c
> index 99e2d1c..cbbf99a 100644
> --- a/dlls/user32/dde_client.c
> +++ b/dlls/user32/dde_client.c
> @@ -632,6 +632,15 @@ static WDML_XACT* WDML_ClientQueueExecute(WDML_CONV* pConv, LPVOID pData, DWORD
> TRACE("XTYP_EXECUTE transaction\n");
>
> - if (pData == NULL)
> - {
> -
>    if (cbData == (DWORD)-1)
> -
>        pConv->instance->lastError = DMLERR_INVALIDPARAMETER;
> -
>    else
> -
>        pConv->instance->lastError = DMLERR_MEMORY_ERROR;
> -
>    return NULL;
> - }

Alexandre suggested to use the existing 'if (cbData == (DWORD)-1)' block,
not introduce a new one.
> @@ -1154,13 +1163,10 @@ HDDEDATA WINAPI DdeClientTransaction(LPBYTE pData, DWORD cbData, HCONV hConv, HS
>     switch (wType)
>     {
>     case XTYP_EXECUTE:
> -
>    /* Windows simply ignores hszItem and wFmt in this case */
> - if (pData == NULL)
> - {
> -
> pConv->instance->lastError = DMLERR_INVALIDPARAMETER;
> -
> return 0;
> - }
> - /* Windows simply ignores hszItem and wFmt in this case */
>
> pXAct = WDML_ClientQueueExecute(pConv, pData, cbData);
> - if (pXAct == NULL)
> -
>    return 0;
>
> break;

The formatting of the above block is strange. Set the tab size in your
editor to 8 and you will see the mess.

user32: Resend Check for NULL pData in DdeClientTransAction expectingto be passed a handle

[Jeff Latimer]

Dmitry Timoshkov wrote:
> > - if (pData == NULL)
> > - {
> > -
> >    if (cbData == (DWORD)-1)
> > -
> >        pConv->instance->lastError = DMLERR_INVALIDPARAMETER;
> > -
> >    else
> > -
> >        pConv->instance->lastError = DMLERR_MEMORY_ERROR;
> > -
> >    return NULL;
> > - }
>
> Alexandre suggested to use the existing 'if (cbData == (DWORD)-1)' block,
> not introduce a new one.

True, though when looking how we handle invalid parameters in most other 
code, the check is upfront where possible and return is as quickly as 
possible.  In this case it looks a lot cleaner and more understandable 
to put in the extra block rather than to work it into the logic.  It 
looks like more elses and I would have to handle the  
WDML_AllocTransaction as it is not be needed if pData was NULL.  It 
seems to complicate the logic and I don't see the benefit.
> The formatting of the above block is strange. Set the tab size in your
> editor to 8 and you will see the mess.

True, I generally use 4 for the tabstop but I can fix that.

Jeff

user32: Resend Check for NULL pData in DdeClientTransAction expectingto be passed a handle

[Jeff Latimer]

Dmitry Timoshkov wrote:
> > - if (pData == NULL)
> > - {
> > -
> >    if (cbData == (DWORD)-1)
> > -
> >        pConv->instance->lastError = DMLERR_INVALIDPARAMETER;
> > -
> >    else
> > -
> >        pConv->instance->lastError = DMLERR_MEMORY_ERROR;
> > -
> >    return NULL;
> > - }
>
> Alexandre suggested to use the existing 'if (cbData == (DWORD)-1)' block,
> not introduce a new one.

True, though when looking how we handle invalid parameters in most other
code, the check is upfront where possible and return is as quickly as
possible.  In this case it looks a lot cleaner and more understandable
to put in the extra block rather than to work it into the logic.  It
looks like more elses and I would have to handle the
WDML_AllocTransaction as it is not be needed if pData was NULL.  It
seems to complicate the logic and I don't see the benefit.
> The formatting of the above block is strange. Set the tab size in your
> editor to 8 and you will see the mess.

True, I generally use 4 for the tabstop but I can fix that.

Jeff