Last week I sent the following patches that add DECIMAL support for OLEAUT32: 1) Tests for all missing functionality: http://www.winehq.org/pipermail/wine-patches/2005-September/020849.html 2) Call VarDecDiv and VarDecSub from variant.c: http://www.winehq.org/pipermail/wine-patches/2005-September/020850.html 3) Correct scaling of high 32 bits of DECIMAL: http://www.winehq.org/pipermail/wine-patches/2005-September/020851.html 4) Add full VarBstrFromDec support: http://www.winehq.org/pipermail/wine-patches/2005-September/020852.html 5) Add full VarDecMul support: http://www.winehq.org/pipermail/wine-patches/2005-September/020853.html 6) Add full VarDecDiv support: http://www.winehq.org/pipermail/wine-patches/2005-September/020854.html
Of these, only patches 2 and 3 have been commited.
Could somebody explain (especially Mr. Alexandre Julliard) whether these patches have been noticed? If so, what can be done to improve them?
Alex Villacís Lasso
Alex Villacís Lasso a_villacis@palosanto.com writes:
Last week I sent the following patches that add DECIMAL support for OLEAUT32:
- Tests for all missing functionality:
http://www.winehq.org/pipermail/wine-patches/2005-September/020849.html 2) Call VarDecDiv and VarDecSub from variant.c: http://www.winehq.org/pipermail/wine-patches/2005-September/020850.html 3) Correct scaling of high 32 bits of DECIMAL: http://www.winehq.org/pipermail/wine-patches/2005-September/020851.html 4) Add full VarBstrFromDec support: http://www.winehq.org/pipermail/wine-patches/2005-September/020852.html 5) Add full VarDecMul support: http://www.winehq.org/pipermail/wine-patches/2005-September/020853.html 6) Add full VarDecDiv support: http://www.winehq.org/pipermail/wine-patches/2005-September/020854.html
Of these, only patches 2 and 3 have been commited.
Could somebody explain (especially Mr. Alexandre Julliard) whether these patches have been noticed? If so, what can be done to improve them?
The main problem I see is you are doing everything with bytes; it would seem more natural and much more efficient to work with 32-bit ints.
Also you are using malloc which should be avoided. If you really need to allocate memory you can use HeapAlloc, but since all the numbers are fixed size I don't think you should need allocations at all, stack buffers should work fine.
Alexandre Julliard wrote:
Alex Villacís Lasso a_villacis@palosanto.com writes:
Last week I sent the following patches that add DECIMAL support for OLEAUT32:
- Tests for all missing functionality:
http://www.winehq.org/pipermail/wine-patches/2005-September/020849.html 2) Call VarDecDiv and VarDecSub from variant.c: http://www.winehq.org/pipermail/wine-patches/2005-September/020850.html 3) Correct scaling of high 32 bits of DECIMAL: http://www.winehq.org/pipermail/wine-patches/2005-September/020851.html 4) Add full VarBstrFromDec support: http://www.winehq.org/pipermail/wine-patches/2005-September/020852.html 5) Add full VarDecMul support: http://www.winehq.org/pipermail/wine-patches/2005-September/020853.html 6) Add full VarDecDiv support: http://www.winehq.org/pipermail/wine-patches/2005-September/020854.html
Of these, only patches 2 and 3 have been commited.
Could somebody explain (especially Mr. Alexandre Julliard) whether these patches have been noticed? If so, what can be done to improve them?
The main problem I see is you are doing everything with bytes; it would seem more natural and much more efficient to work with 32-bit ints.
Also you are using malloc which should be avoided. If you really need to allocate memory you can use HeapAlloc, but since all the numbers are fixed size I don't think you should need allocations at all, stack buffers should work fine.
I am a paranoid about endianness. I wanted absolute control over the layout of the significant bits, and working with bytes instead of 32-bit ints allowed me to port the algorithms more easily, in additon to being capable of using memmove for bit shifting. Why is malloc() evil even if it is only used internally and its memory is freed before returning the results?
Both this and the malloc issue can be fixed. However, I don't see why this should prevent the test patch from being submitted, unless there is a policy about not allowing tests that do not yet pass with current code.
Alex Villacís Lasso
On Mon, 26 Sep 2005, Alex Villacís Lasso wrote: [...]
Why is malloc() evil even if it is only used internally and its memory is freed before returning the results?
I think this is because it introduces a dependency on msvcrt.dll on the Windows and ReactOS platforms. So it's better to stick to 'pure' Win32 APIs as much as possible.
Alex Villacís Lasso a_villacis@palosanto.com writes:
Both this and the malloc issue can be fixed. However, I don't see why this should prevent the test patch from being submitted, unless there is a policy about not allowing tests that do not yet pass with current code.
Yes, there is such a policy. All the tests must always pass in CVS, otherwise the test suite is useless. If you want the test to get in before the code that's certainly possible, but you have to add todo_wine around all the failing tests.
Alexandre Julliard wrote:
Alex Villacís Lasso a_villacis@palosanto.com writes:
Both this and the malloc issue can be fixed. However, I don't see why this should prevent the test patch from being submitted, unless there is a policy about not allowing tests that do not yet pass with current code.
Yes, there is such a policy. All the tests must always pass in CVS, otherwise the test suite is useless. If you want the test to get in before the code that's certainly possible, but you have to add todo_wine around all the failing tests.
I sent the patches again with the required changes (use DWORDs instead of bytes where possible, refrain from using malloc()) on October 3. However, there has been no response about them. Has anybody noticed the new patches, and if so, what new changes should be made? Or are they being rejected because of the code freeze?
Alex Villacís Lasso
Alex Villacís Lasso a_villacis@palosanto.com writes:
I sent the patches again with the required changes (use DWORDs instead of bytes where possible, refrain from using malloc()) on October 3. However, there has been no response about them. Has anybody noticed the new patches, and if so, what new changes should be made? Or are they being rejected because of the code freeze?
The patches look good, but yes, they are too large to get in during the code freeze.
-
Alex Villacís Lasso -
Alexandre Julliard -
Francois Gouget