Re: [try2] kernel32: Add partial stub for NeedCurrentDirectoryForExePath - what's wrong?

[Kirill K. Smirnov]

> On 4/24/07, Kirill K. Smirnov <lich(a)math.spbu.ru> wrote:
> > Hi,
> >   I've sent the patch:
> > http://www.winehq.com/pipermail/wine-patches/2007-April/038205.html
> > about a week ago and it has not been applied. Please, explain, what is
> > wrong with it?
>
> You haven't sent in any tests for this function.  Is there some app
> that needs it?
> I've mentioned it in the first try: windows 2003 cmd.exe.
> +    static const WCHAR env_name[] =
> {'N','o','D','e','f','a','u','l','t','C','u','r','r','e','n','t','D','i','r
> ','e','c','t','o','r','y','I','n','E','x','e','P','a','t','h',0};
>
> That's a really long line.  We try to keep the lines to a length of 80.
> Hm... I've improve it.
> +    /* Check name first */
> +    if (strchrW(name, '\\'))
> +        return TRUE;
>
> You should probably check name for NULL, but you'll have to test how
> Windows handles this.  That's a pretty bad comment too; check name for
> what?
> For containing backshash. Yes, comment must be better.

Thanks,
--
Kirill

[try2] kernel32: Add partial stub for NeedCurrentDirectoryForExePath - what's wrong?

[James Hawkins]

On 4/25/07, Kirill K. Smirnov <lich(a)math.spbu.ru> wrote:
> > On 4/24/07, Kirill K. Smirnov <lich(a)math.spbu.ru> wrote:
> > > Hi,
> > >   I've sent the patch:
> > > http://www.winehq.com/pipermail/wine-patches/2007-April/038205.html
> > > about a week ago and it has not been applied. Please, explain, what is
> > > wrong with it?
> >
> > You haven't sent in any tests for this function.  Is there some app
> > that needs it?
> > I've mentioned it in the first try: windows 2003 cmd.exe.
> > +    static const WCHAR env_name[] =
> > {'N','o','D','e','f','a','u','l','t','C','u','r','r','e','n','t','D','i','r
> > ','e','c','t','o','r','y','I','n','E','x','e','P','a','t','h',0};
> >
> > That's a really long line.  We try to keep the lines to a length of 80.
> > Hm... I've improve it.
> > +    /* Check name first */
> > +    if (strchrW(name, '\\'))
> > +        return TRUE;
> >
> > You should probably check name for NULL, but you'll have to test how
> > Windows handles this.  That's a pretty bad comment too; check name for
> > what?
> > For containing backshash. Yes, comment must be better.

A comment that states you're looking for a backslash is superfluous;
the code makes that obvious.  A better comment would be 'check for
filename only' or something along those lines.  How do you know that's
what this function does if you haven't tested it?

-- 
James Hawkins