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@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@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
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
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
-
Christian Costa -
Robert Shearman