Re: [PATCH 5/5] reg: Simplify syntax and help logic

[Alexandre Julliard]

Hugh McMaster  writes:
> @@ -907,19 +915,16 @@ int wmain(int argc, WCHAR *argvW[])
>          return 0;
>      }
> - option_help = (!lstrcmpW(argvW[2], slashHelpW) || !lstrcmpiW(argvW[2], slashHW));

You should check argc first.
> -
>    if (argc < 3)
> -
>    {
> -
>        output_message(STRING_INVALID_SYNTAX);
> -
>        output_message(STRING_FUNC_HELP, struprW(argvW[1]));
> -
>        return 1;
> -
>    }
> -
>    else if (argc == 3 && (!lstrcmpW(argvW[2], slashHelpW) ||
> -
>                           !lstrcmpiW(argvW[2], slashHW)))
> -
>    if (argc < 3 || (argc > 3 && option_help))
> -
>        return invalid_syntax(argvW[1]);
> -
>    else if (option_help)
>    {

This doesn't feel like much of an improvement. You could probably come
up with more useful helper functions.

[PATCH 5/5] reg: Simplify syntax and help logic

[Hugh McMaster]

On Tuesday, 24 May 2016 11:35 PM, Alexandre Julliard wrote:
> Hugh McMaster writes:
> > @@ -907,19 +915,16 @@ int wmain(int argc, WCHAR *argvW[])
> >          return 0;
> >      }
> > - option_help = (!lstrcmpW(argvW[2], slashHelpW) || !lstrcmpiW(argvW[2], slashHW));
>
> You should check argc first.

Ah, yes, I missed that.
> > -
> >    if (argc < 3)
> > -
> >    {
> > -
> >        output_message(STRING_INVALID_SYNTAX);
> > -
> >        output_message(STRING_FUNC_HELP, struprW(argvW[1]));
> > -
> >        return 1;
> > -
> >    }
> > -
> >    else if (argc == 3 && (!lstrcmpW(argvW[2], slashHelpW) ||
> > -
> >                           !lstrcmpiW(argvW[2], slashHW)))
> > -
> >    if (argc < 3 || (argc > 3 && option_help))
> > -
> >        return invalid_syntax(argvW[1]);
> > -
> >    else if (option_help)
> >    {
>
> This doesn't feel like much of an improvement. You could probably come
> up with more useful helper functions.

Ideally, I'd like to move all of that logic out of the 'if' block string comparisons.
One major issue I see is that users may pass invalid operation names, e.g. abc,
so we need to ensure that we only continue with valid strings.

I believe this will also cause problems when functions like sane_path() are moved
into wmain() as well, so it's better to deal with this now.

One solution is to iterate over an array of WCHAR * and return a BOOL indicating
whether the string is valid or not.

As for passing output_message() the correct help string ID, leaving that code where
it is now is probably best, particularly if I can use the BOOL option_help. If not, we'd
have to look up the message ID information in a function or a struct. I'm not if you
think this change would have any benefit.