Re: (4th) Janitorial dlls/advapi32/registry.c W->A cleanup

[Alexandre Julliard]

Tony Lambregts <tony_lambregts(a)telusplanet.net> writes:
> +    len = WideCharToMultiByte(CP_ACP, 0, nameW, -1, NULL, 0, NULL, NULL);
> +    nameA = HeapAlloc(GetProcessHeap(), 0, len);
> +    WideCharToMultiByte(CP_ACP, 0, nameW, -1, nameA, len, NULL, NULL);
> +    
>      for (;;)
>      {
> -        sprintf( name, "reg%04x.tmp", count++ );
> -        handle = CreateFileA( buffer, GENERIC_WRITE, 0, NULL,
> +        sprintf( nameA, "reg%04x.tmp", count++ );
> +        handle = CreateFileW( buffer, GENERIC_WRITE, 0, NULL,

This can't work, the sprintf is supposed to modify the buffer, not a
copy of it.

Also note that registry functions are not supposed to set last error,
so you probably don't want to use Dmitry's wrapper here.

-- 
Alexandre Julliard
julliard(a)winehq.com

(4th) Janitorial dlls/advapi32/registry.c W->A cleanup

[Dan Kegel]

Alexandre Julliard wrote:
> This can't work, the sprintf is supposed to modify the buffer, not a
> copy of it.
>
> Also note that registry functions are not supposed to set last error,
> so you probably don't want to use Dmitry's wrapper here.

Hmm.  Would it be possible for janitors to as a matter
of course submit a regression test program for the
routines they are sweeping up?  That might avoid a few of these
kind of issues.
- Dan

-- 
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045

(4th) Janitorial dlls/advapi32/registry.c W->A cleanup

[Tony Lambregts]

Dan Kegel wrote:
> Alexandre Julliard wrote:
> > This can't work, the sprintf is supposed to modify the buffer, not a
> > copy of it.
> >
> > Also note that registry functions are not supposed to set last error,
> > so you probably don't want to use Dmitry's wrapper here.
>
> Hmm.  Would it be possible for janitors to as a matter
> of course submit a regression test program for the
> routines they are sweeping up?  That might avoid a few of these
> kind of issues.
> - Dan

I don't agree. The way I see it these janitorial projects are supposed 
grunt work, and a lot of these W->A changes are just that. They are just 
simple changes that anyone can do. Making it (writing the test) a 
requirement might raise the bar to the point where no one (else) will 
even attempt to do this stuff.

I was not comfortable with the sprintf situation when I wrote it and I 
suspected it was wrong. The best way, that I know of,  to get these 
problems resolved is to get someone else to look at them, and submitting 
a patch is the best way to get the feedback I need. The bottom line is 
that, even if we had conformance tests I would still be up against the 
sprintf/swprintf problem.

OTOH I was going down the list of W->A calls and tried to work 
on dlls/advapi32/crypt.c  It has no tests and the amount of problems 
with it hurt my brain. I have no intention of going any further with it 
without some conformance tests. Even with conformance tests, I have my 
doubts about being able to do without at least some feedback.

IMO after the simple changes are done, some of these W->A projects (like 
crypt.c) are going to require some detailed knowledge. Exactly what to 
do with them I don't know but I would not think of them as simply 
janitorial.

I think it would be a better idea to have adding missing tests as a 
separate janitorial project. Perhaps Patrik Stridvall has a inclination 
to help out here. (So we have a list)


-- 

Tony Lambregts

(4th) Janitorial dlls/advapi32/registry.c W->A cleanup

[Tony Lambregts]

Alexandre Julliard wrote:
> Tony Lambregts <tony_lambregts(a)telusplanet.net> writes:
> > +    len = WideCharToMultiByte(CP_ACP, 0, nameW, -1, NULL, 0, NULL, NULL);
> > +    nameA = HeapAlloc(GetProcessHeap(), 0, len);
> > +    WideCharToMultiByte(CP_ACP, 0, nameW, -1, nameA, len, NULL, NULL);
> > +    
> >     for (;;)
> >     {
> > -        sprintf( name, "reg%04x.tmp", count++ );
> > -        handle = CreateFileA( buffer, GENERIC_WRITE, 0, NULL,
> > +        sprintf( nameA, "reg%04x.tmp", count++ );
> > +        handle = CreateFileW( buffer, GENERIC_WRITE, 0, NULL,
>
> This can't work, the sprintf is supposed to modify the buffer, not a
> copy of it.

It didn't feel right to me. I had a problem with this in the first place 
since I could not use swprintf (at least not without including msvcrt.h) 
My poor brain needs more advice.
> Also note that registry functions are not supposed to set last error,
> so you probably don't want to use Dmitry's wrapper here.

OK I can do this without the wrapper thats not a problem. I still need 
help with the sprintf problem though.

-- 

Tony Lambregts