"Maarten Lankhorst" m.b.lankhorst@gmail.com writes:
- TRACE("Flags: %08x, recipients: %p(0x%x), msg: %04x, wparam: %08lx, lparam: %08lx\n", flags, recipients,
(recipients ? *recipients : recips), msg, wp, lp);- if (flags > BSF_LUID)
It doesn't make sense to compare flags with >. What are you trying to check for?
-}
- else
FIXME("Recipients %08x not supported!\n", *recipients);- if (ret > 0 && !GetLastError())
SetLastError(lasterror);
In general if you have to save and restore last error you are doing something wrong.
Hello Alexandre,
2008/3/24, Alexandre Julliard julliard@winehq.org:
"Maarten Lankhorst" m.b.lankhorst@gmail.com writes:
- TRACE("Flags: %08x, recipients: %p(0x%x), msg: %04x, wparam: %08lx, lparam: %08lx\n", flags, recipients,
(recipients ? *recipients : recips), msg, wp, lp);- if (flags > BSF_LUID)
It doesn't make sense to compare flags with >. What are you trying to check for?
-}
- else
FIXME("Recipients %08x not supported!\n", *recipients);- if (ret > 0 && !GetLastError())
SetLastError(lasterror);In general if you have to save and restore last error you are doing something wrong.
Or wine is doing something wrong. After some more digging I found that SetLastError() was set to 0 by TlsGetValue when called from X11DRV's MsgWaitForMultipleObjectsEx. After I tried fixing this so that SetLastError is only set when NULL is returned, one of the other tests miraculously started working inside a todo block too (cursoricon). I'll work on some testcases to verify that tlsgetvalue only calls SetLastError(0) when succesfully returning null.
Cheers, Maarten.
"Maarten Lankhorst" m.b.lankhorst@gmail.com writes:
Or wine is doing something wrong. After some more digging I found that SetLastError() was set to 0 by TlsGetValue when called from X11DRV's MsgWaitForMultipleObjectsEx. After I tried fixing this so that SetLastError is only set when NULL is returned, one of the other tests miraculously started working inside a todo block too (cursoricon). I'll work on some testcases to verify that tlsgetvalue only calls SetLastError(0) when succesfully returning null.
In most cases it's the test that is doing something wrong by being too strict. We don't care whether last error is modified or not on success, and there's no reason to test for it, unless it's one of the very few functions that do something special on success, or unless there is a real app that depends on it.
Alexandre Julliard wrote:
"Maarten Lankhorst" m.b.lankhorst@gmail.com writes:
Or wine is doing something wrong. After some more digging I found that SetLastError() was set to 0 by TlsGetValue when called from X11DRV's MsgWaitForMultipleObjectsEx. After I tried fixing this so that SetLastError is only set when NULL is returned, one of the other tests miraculously started working inside a todo block too (cursoricon). I'll work on some testcases to verify that tlsgetvalue only calls SetLastError(0) when succesfully returning null.
In most cases it's the test that is doing something wrong by being too strict. We don't care whether last error is modified or not on success, and there's no reason to test for it, unless it's one of the very few functions that do something special on success, or unless there is a real app that depends on it.
I remember sending the patch for this exact thing a while back, which you of course refused under the same pretense - showing a real life app that depends on it. So if tests are not a real life app that shows a difference between Wine and windows - then why do we bother with Wine at all? At least should change the moto to "might run some apps with loads of problems and never be 100% compatible".
One for sure will find an app that in one way or another depends on undocumented windows behavior. So if a test can be written to show a difference between Wine and windows. Then this difference should be fixed in Wine.
Vitaliy.
Vitaliy Margolen wine-devel@kievinfo.com writes:
I remember sending the patch for this exact thing a while back, which you of course refused under the same pretense - showing a real life app that depends on it. So if tests are not a real life app that shows a difference between Wine and windows - then why do we bother with Wine at all? At least should change the moto to "might run some apps with loads of problems and never be 100% compatible".
One for sure will find an app that in one way or another depends on undocumented windows behavior. So if a test can be written to show a difference between Wine and windows. Then this difference should be fixed in Wine.
It all comes down to where we want to spend our efforts. It's very easy to write a million last error tests, and then we'd have to spend months fixing Wine by adding save/restore of last error all over the place, for no benefit at all. Our time is much better spent fixing things that actually matter, and not making the code uglier for things that don't.
Alexandre Julliard wrote:
Vitaliy Margolen wine-devel@kievinfo.com writes:
I remember sending the patch for this exact thing a while back, which you of course refused under the same pretense - showing a real life app that depends on it. So if tests are not a real life app that shows a difference between Wine and windows - then why do we bother with Wine at all? At least should change the moto to "might run some apps with loads of problems and never be 100% compatible".
One for sure will find an app that in one way or another depends on undocumented windows behavior. So if a test can be written to show a difference between Wine and windows. Then this difference should be fixed in Wine.
It all comes down to where we want to spend our efforts. It's very easy to write a million last error tests, and then we'd have to spend months fixing Wine by adding save/restore of last error all over the place, for no benefit at all. Our time is much better spent fixing things that actually matter, and not making the code uglier for things that don't.
But that's the thing. How can you definitively say that there isn't a program that affected by this? And this sort of difference might affect number of programs in a ways that really hard to catch.
Vitaliy.
Vitaliy Margolen wine-devel@kievinfo.com writes:
But that's the thing. How can you definitively say that there isn't a program that affected by this?
Of course you can never be certain, but after having debugged a number of apps, and checked the behavior of APIs across Windows versions, there are things that you know do matter, and things that don't; and the last error behavior on success is definitely in the latter category.
Yes, I'm sure someone someday can find a case where some specific app depends on some specific function to handle last error in a different way; but there's no reason to believe that BroadcastSystemMessage is more likely to trigger that bug than any other of the 10,000 functions that touch last error. So we either wait until we find an app that depends on this and fix the one offending function, or we spend months adding ugly code to every single API; adding the ugly code to only a handful of randomly chosen functions doesn't make any sense.
Hello Alexandre,
2008/3/31, Alexandre Julliard julliard@winehq.org:
Vitaliy Margolen wine-devel@kievinfo.com writes:
But that's the thing. How can you definitively say that there isn't a program that affected by this?
Of course you can never be certain, but after having debugged a number of apps, and checked the behavior of APIs across Windows versions, there are things that you know do matter, and things that don't; and the last error behavior on success is definitely in the latter category.
Yes, I'm sure someone someday can find a case where some specific app depends on some specific function to handle last error in a different way; but there's no reason to believe that BroadcastSystemMessage is more likely to trigger that bug than any other of the 10,000 functions that touch last error. So we either wait until we find an app that depends on this and fix the one offending function, or we spend months adding ugly code to every single API; adding the ugly code to only a handful of randomly chosen functions doesn't make any sense.
Ok, I'll remove the last error tests where they don't matter then once I clean up my git tree a little, there's too much stuff lying around at the moment. :-)
Cheers, Maarten.
Hello Alexandre,
2008/3/30, Alexandre Julliard julliard@winehq.org:
"Maarten Lankhorst" m.b.lankhorst@gmail.com writes:
Or wine is doing something wrong. After some more digging I found that SetLastError() was set to 0 by TlsGetValue when called from X11DRV's MsgWaitForMultipleObjectsEx. After I tried fixing this so that SetLastError is only set when NULL is returned, one of the other tests miraculously started working inside a todo block too (cursoricon). I'll work on some testcases to verify that tlsgetvalue only calls SetLastError(0) when succesfully returning null.
In most cases it's the test that is doing something wrong by being too strict. We don't care whether last error is modified or not on success, and there's no reason to test for it, unless it's one of the very few functions that do something special on success, or unless there is a real app that depends on it.
Well, EnumWindows encourages applications to use setlasterror to set an error message to pass a return value. Suppose if I took this approach, and then called SendMessageTimeout from my enumeration function, the last error would be useless because it's overwritten. Adding a few lines to SendMessageTimeout confirms this behavior: Windows doesn't overwrite the last error by default. I tracked where it does timeout to that tlsgetvalue call in MsgWaitForMultipleObjectsEx, which doesn't currently have last error tests. I assume it doesn't change last error unless it returns null and has success. This would fix SendMessageTimeout to act more like windows and my BroadcastSystemMessage implementation.
And regarding 'there is no real life app that depends on it', too often I find obscure problems with applications because wine does something slightly different then windows does...
Cheers, Maarten.
-
Alexandre Julliard -
Maarten Lankhorst -
Vitaliy Margolen