Re: [PATCH] shell32: Check the return value of DoPaste

[Huw Davies]

On Wed, May 30, 2018 at 02:28:03AM +0000, Alistair Leslie-Hughes wrote:
> Based of patch by Michael Müller.
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/shell32/shlview_cmenu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/shell32/shlview_cmenu.c b/dlls/shell32/shlview_cmenu.c
> index fc761e1..74270b5 100644
> --- a/dlls/shell32/shlview_cmenu.c
> +++ b/dlls/shell32/shlview_cmenu.c
> @@ -1142,7 +1142,8 @@ static BOOL DoPaste(ContextMenu *This)
>          /* do the copy/move */
>          if (psfhlpdst && psfhlpsrc)
>          {
> -
>        ISFHelper_CopyItems(psfhlpdst, psfFrom, lpcida->cidl, (LPCITEMIDLIST*)apidl);
> -
>          if (SUCCEEDED(ISFHelper_CopyItems(psfhlpdst, psfFrom, lpcida->cidl, (LPCITEMIDLIST*)apidl)))
> -
>              bSuccess = TRUE;
> /* FIXME handle move
>   ISFHelper_DeleteItems(psfhlpsrc, lpcida->cidl, apidl);
>   */

...
> @@ -1239,7 +1241,8 @@ static HRESULT WINAPI BackgroundMenu_InvokeCommand(
>                  break;
>          case FCIDM_SHVIEW_INSERT:
> -
>            DoPaste(This);
> -
>            if (!DoPaste(This))
> -
>                hr = E_FAIL;

DoPaste should return HRESULT.  When you have to have constructs like
this, it's clear the error handling isn't right.

However, before we start changing DoPaste, I'd suggest moving it
further up the file to avoid the declaration that gets added in
a later patch.  As part of the move, the 'interesting' indentation
of this function could be fixed and some of the variable names
could be changed to be more meaningful.  Then we can start fixing
things.

Huw.