Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> +char* CDECL _get_narrow_winmain_command_line(void)
> +{
> - static char *narrow_command_line;
> - char *s;
> -
> - if (narrow_command_line)
> -
>  return narrow_command_line;
> -
> - s = GetCommandLineA();
> - if (*s == '"')
> - {
> -
>  /* skip all leading quotes */
> -
>  while (*++s == '"')
> -
>      ;
> -
>  /* skip everything up to next quote */
> -
>  while (*s && *s++ != '"')
> -
>      ;
> - }
> - else
> - {
> -
>  while (*s && *s != ' ' && *s != '\t')
> -
>  {
> -
>      /* allow embedded quoted blocks */
> -
>      if (*s == '"')
> -
>      {
> -
>          s++;
> -
>          while (*s && *s++ != '"')
> -
>             ;
> -
>      }
> -
>      else
> -
>          s++;
> -
>  }
> - }
> - while (*s == ' ' || *s == '\t')
> -
>  s++;
> -
> - return narrow_command_line = s;
>
> +}

Once again:
Where did you get an idea of such an implementation? Is this copied from
some place or based on some other implementation? Why for instance simple
'return GetCommandLineA()' is not enough as an initial implementation?

[v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

[Piotr Caban]

On 07/26/16 04:32, Dmitry Timoshkov wrote:
> Once again:
> Where did you get an idea of such an implementation? Is this copied from
> some place or based on some other implementation? Why for instance simple
> 'return GetCommandLineA()' is not enough as an initial implementation?

Probably I'm just stating obvious but the function skips executable name 
from the command line. That's why returning what GetCommandLineA returns 
is incorrect.

Thanks,
Piotr

[v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

[Dmitry Timoshkov]

Piotr Caban  wrote:
> > Once again:
> > Where did you get an idea of such an implementation? Is this copied from
> > some place or based on some other implementation? Why for instance simple
> > 'return GetCommandLineA()' is not enough as an initial implementation?
>
> Probably I'm just stating obvious but the function skips executable name 
> from the command line. That's why returning what GetCommandLineA returns 
> is incorrect.
> From what documentation/description is that obvious? That's not a common

practice to send patches for not very obvious things without any tests.
Why this implementation treats specially tabs and spaces but not other
white space characters? Is that also obvious? How about the tests for
quotes/embedded quotes?

[v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

[Piotr Caban]

On 07/27/16 15:29, Dmitry Timoshkov wrote:
> Piotr Caban  wrote:
> > > Once again:
> > > Where did you get an idea of such an implementation? Is this copied from
> > > some place or based on some other implementation? Why for instance simple
> > > 'return GetCommandLineA()' is not enough as an initial implementation?
> >
> > Probably I'm just stating obvious but the function skips executable name
> > from the command line. That's why returning what GetCommandLineA returns
> > is incorrect.
>
> From what documentation/description is that obvious? That's not a common
> practice to send patches for not very obvious things without any tests.
> Why this implementation treats specially tabs and spaces but not other
> white space characters? Is that also obvious? How about the tests for
> quotes/embedded quotes?

I was not asking Nikolay for the tests because I think it's not easy to 
write them. Some interesting tests needs executable name with ' ' in its 
name. I also don't like the idea of starting many processes just to test 
the function. On the other hand the initial test executable may be 
started in many different ways (like e.g. passing first argument quoted 
makes it impossible to check the return with simple comparison).

That doesn't mean that I'm against adding a test or 2 for that. Anyway I 
think this implementation is good after doing some tests myself.

[v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

[Dmitry Timoshkov]

Piotr Caban  wrote:
> > > > Once again:
> > > > Where did you get an idea of such an implementation? Is this copied from
> > > > some place or based on some other implementation? Why for instance simple
> > > > 'return GetCommandLineA()' is not enough as an initial implementation?
> > >
> > > Probably I'm just stating obvious but the function skips executable name
> > > from the command line. That's why returning what GetCommandLineA returns
> > > is incorrect.
> >
> > From what documentation/description is that obvious? That's not a common
> > practice to send patches for not very obvious things without any tests.
> > Why this implementation treats specially tabs and spaces but not other
> > white space characters? Is that also obvious? How about the tests for
> > quotes/embedded quotes?
>
> I was not asking Nikolay for the tests because I think it's not easy to 
> write them. Some interesting tests needs executable name with ' ' in its 
> name. I also don't like the idea of starting many processes just to test 
> the function. On the other hand the initial test executable may be 
> started in many different ways (like e.g. passing first argument quoted 
> makes it impossible to check the return with simple comparison).
>
> That doesn't mean that I'm against adding a test or 2 for that. Anyway I 
> think this implementation is good after doing some tests myself.

Testing on your own is good, however that doesn't count like an answer
to my questions above which still left unanswered. If Wine development
would follow this kind of quality control it wouldn't get that far as
it is now.