Re: [PATCH] shell32: Implement IShellDispatch2::ShellExecute

[Dmitry Timoshkov]

Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> +    TRACE("(%s, %s, %s, %s, %s): stub\n", debugstr_w(file), debugstr_variant(&v_args),
> +            debugstr_variant(&v_dir), debugstr_variant(&v_op), debugstr_variant(&v_show));

Probably ": stub" part should be removed.
> +    if (V_VT(&v_op) == VT_BSTR)
> +        op = V_BSTR(&v_op);
> +
> +    if (V_VT(&v_args) == VT_BSTR)
> +        args = V_BSTR(&v_args);
> +
> +    if (V_VT(&v_dir) == VT_BSTR)
> +        dir = V_BSTR(&v_dir);
> +
> +    if (V_VT(&v_show) == VT_INT)
> +        show = V_INT(&v_show);
> +
> +    ShellExecuteW(NULL, op, file, args, dir, show);
> +
> +    return S_OK;

Shouldn't return value somehow reflect what ShellExecute returns?
Also as usually it would be nice to see some tests for this, especially
with not handled variant types.

-- 
Dmitry.

[PATCH] shell32: Implement IShellDispatch2::ShellExecute

[Andrew Eikum]

On Fri, Oct 16, 2015 at 11:28:27PM +0800, Dmitry Timoshkov wrote:
> Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> > +    TRACE("(%s, %s, %s, %s, %s): stub\n", debugstr_w(file), debugstr_variant(&v_args),
> > +            debugstr_variant(&v_dir), debugstr_variant(&v_op), debugstr_variant(&v_show));
>
> Probably ": stub" part should be removed.

Yeah, it should.
> > +    if (V_VT(&v_op) == VT_BSTR)
> > +        op = V_BSTR(&v_op);
> > +
> > +    if (V_VT(&v_args) == VT_BSTR)
> > +        args = V_BSTR(&v_args);
> > +
> > +    if (V_VT(&v_dir) == VT_BSTR)
> > +        dir = V_BSTR(&v_dir);
> > +
> > +    if (V_VT(&v_show) == VT_INT)
> > +        show = V_INT(&v_show);
> > +
> > +    ShellExecuteW(NULL, op, file, args, dir, show);
> > +
> > +    return S_OK;
>
> Shouldn't return value somehow reflect what ShellExecute returns?
> Also as usually it would be nice to see some tests for this, especially
> with not handled variant types.

I wanted to avoid re-testing all of ShellExecute itself and verifying
the errors, but it's a good idea to test bad variants. I'll see what I
can do.

Thanks,
Andrew