Re: wine/dlls shell32/shlfileop.c shell32/shlexec. ...

[Christian Costa]

Hi Michael,

This is typically the kind of patches that breaks everything.

The code you find useless, is often use in lazy allocation or error 
handling.
I doubt the current wine code is so bad designed even if there is some 
cases where the test is useless.
For example in memallocator.c, this is wrong because you can release the 
allocator even before allocating
the memory with the SetProperties method.
Looking at your other patches, there is some changes that seems suspect 
to me (in winmm and directx).

I think, this kind of cleanup should be done very carefully by 
understanding what the code really does.
Morever, sending large patches that involves several dll makes it 
appears as obvious cleanups which they are not.

Bye,
Christian

PS: Alexandre, please, could you remove all theses patches ?

Alexandre Julliard wrote:
> ChangeSet ID:	15010
> CVSROOT:	/opt/cvs-commit
> Module name:	wine
> Changes by:	julliard(a)wine.codeweavers.com	2004/12/23 11:12:07
>
> Modified files:
>   dlls/shell32   : shlfileop.c shlexec.c shelllink.c 
>                    shell32_main.c pidl.c brsfolder.c 
>                    autocomplete.c 
>   dlls/setupapi  : setupx_main.c parser.c 
>   dlls/serialui  : confdlg.c 
>   dlls/rsaenh    : rsaenh.c handle.c 
>   dlls/rpcrt4    : rpc_server.c rpc_message.c rpc_binding.c 
>   dlls/richedit  : reader.c 
>   dlls/quartz    : memallocator.c
>
> Log message:
>   Michael Stefaniuc <mstefani(a)redhat.de>
>   Do not check for non NULL pointer before HeapFree'ing it. It's
>   redundant.
>
> Patch: http://cvs.winehq.org/patch.py?id=15010
>
> Old revision  New revision  Changes     Path
> 1.49          1.50          +1 -2       wine/dlls/shell32/shlfileop.c
> 1.58          1.59          +1 -1       wine/dlls/shell32/shlexec.c
> 1.79          1.80          +17 -40     wine/dlls/shell32/shelllink.c
> 1.141         1.142         +2 -2       wine/dlls/shell32/shell32_main.c
> 1.123         1.124         +1 -1       wine/dlls/shell32/pidl.c
> 1.53          1.54          +1 -4       wine/dlls/shell32/brsfolder.c
> 1.11          1.12          +2 -4       wine/dlls/shell32/autocomplete.c
> 1.29          1.30          +3 -3       wine/dlls/setupapi/setupx_main.c
> 1.14          1.15          +1 -2       wine/dlls/setupapi/parser.c
> 1.19          1.20          +1 -2       wine/dlls/serialui/confdlg.c
> 1.7           1.8           +4 -4       wine/dlls/rsaenh/rsaenh.c
> 1.2           1.3           +1 -2       wine/dlls/rsaenh/handle.c
> 1.33          1.34          +1 -1       wine/dlls/rpcrt4/rpc_server.c
> 1.11          1.12          +2 -5       wine/dlls/rpcrt4/rpc_message.c
> 1.29          1.30          +1 -1       wine/dlls/rpcrt4/rpc_binding.c
> 1.18          1.19          +1 -2       wine/dlls/richedit/reader.c
> 1.5           1.6           +1 -2       wine/dlls/quartz/memallocator.c

wine/dlls shell32/shlfileop.c shell32/shlexec. ...

[Robert Shearman]

Christian Costa wrote:
> Hi Michael,
>
> This is typically the kind of patches that breaks everything.

It cannot break anything.
> The code you find useless, is often use in lazy allocation or error 
> handling.
> I doubt the current wine code is so bad designed even if there is some 
> cases where the test is useless.
> For example in memallocator.c, this is wrong because you can release 
> the allocator even before allocating
> the memory with the SetProperties method.

I think you are a little confused. HeapFree can be called with a NULL 
pointer. On Win9x, this causes an error to be set, but from Windows 2000 
onwards and Wine, it is allowed and is not an error.
> Looking at your other patches, there is some changes that seems 
> suspect to me (in winmm and directx).

The new code is perfectly correct, and is cleaner.

Rob

wine/dlls shell32/shlfileop.c shell32/shlexec. ...

[Christian Costa]

Robert Shearman wrote:
> Christian Costa wrote:
> > Hi Michael,
> >
> > This is typically the kind of patches that breaks everything.
>
> It cannot break anything.
> > The code you find useless, is often use in lazy allocation or error 
> > handling.
> > I doubt the current wine code is so bad designed even if there is 
> > some cases where the test is useless.
> > For example in memallocator.c, this is wrong because you can release 
> > the allocator even before allocating
> > the memory with the SetProperties method.
>
> I think you are a little confused. HeapFree can be called with a NULL 
> pointer. On Win9x, this causes an error to be set, but from Windows 
> 2000 onwards and Wine, it is allowed and is not an error.

Ok! I didn't know that. :-(
I feel better now. :-)
> > Looking at your other patches, there is some changes that seems 
> > suspect to me (in winmm and directx).
>
> The new code is perfectly correct, and is cleaner.

Yes it is. :-)
It's true that if this code was so bad, Alexandre would have not 
accepted it.

Bye,
Christian