"Dimitrie O. Paun" wrote:
On December 5, 2002 11:16 am, Alexandre Julliard wrote:
It's the kind of thinking that leads to having an exception handler inside strlen() like Windows does. It's just plain wrong.
<nod/>. This is generally true for other things as well:
Many places in our code where we have invariant like pointer p is never NULL, yet we always have:
if (!p) return NULL; /* or whatever error condition */
Such a check is harmful (hides bugs), slow (useless runtime check), bloat (more generated code), ugly (clutters the code), at the very least. Just Don't Do It (TM) -- let the code crash.
Agreed that it's harmful because it hides what may be a real error. Lousy idea unless p really is allowed to have that value.
If it's a call exposed to external callers it needs to warn somehow, even to the console if necessary (assuming that Wine wants to give end users some feedback they can give back to the Wine developers and to the developer of the faulty application). Those warnings can vanish should there ever be a production build with debugging removed. If it's internal only it should be an assert or fixme.
Being too free with "let the code crash" is a bad idea. Microsoft got a lot of really bad heckling in the Windows 3.1 era for GPFs. Wine doesn't want to crash if it's reasonably harmless to continue (like for a failed screen update). Does want to report the problem, of course, rather than silently ignore it, otherwise it'll never get fixed.
- ZeroMemory when we don't need to, when we pass struct internally, yet same functions can be called externally with no guarantee that the struct has unused field zeroed out.
Agreed if it is a call to an internal only function. If it's to a function with an external API then it may need zeroing to get to the state required for that.
- In general, we have all sort of 'defensive programming' checks for invariants. Problem is, we never going to catch bugs in this area this way. Just use assert() if you feel a check is needed.
If it's a documented external interface it can't assert and die because the application making the call presumably works on real Windows. So it needs to be warned/fixme'd or whatever so end users can report the issue to Wine developers for fixing. If it's internal, assert is sort of OK, though as someone trying to get an application working I want a fixme rather than sudden death (except in file output interfaces!). I don't need perfect functionality. I do need to get the app to the point where it is safely functioning and an assert may kill it when it doesn't need to die.
In short, this defensive programming crap is just that: crap.
Do you agree with the way I've explained how I see the difference between redundant defensive programming and detecting and reporting problems which need to be fixed? If not, where do you differ and why?
James
___________________________________________________ The ALL NEW CS2000 from CompuServe Better! Faster! More Powerful! 250 FREE hours! Sign-on Now! http://www.compuserve.com/trycsrv/cs2000/webmail/
On Mon, 9 Dec 2002 vkxzjq6lg@cswebmail.com wrote: [...]
Being too free with "let the code crash" is a bad idea. Microsoft got a lot of really bad heckling in the Windows 3.1 era for GPFs. Wine doesn't want to crash if it's reasonably harmless to continue (like for a failed screen update). Does want to report the problem, of course, rather than silently ignore it, otherwise it'll never get fixed.
There is a very significant difference between Wine and Windows: if Wine crashes only one process is impacted. And if that process did something wrong then I think it's ok to let it crash. However, if Windows (3.1 or other) crashes (i.e. gives you a GPF or a blue screen of death), then: - all other applications die - all unsaved data of _other_ applications is lost - anything since the last flush to disk is lost - the filesystem may become corrupted
And it is these other effects that make GPFs and blue-screen of death unacceptable. Translated to Wine, this means that it is not acceptable for the wineserver to crash, but it's not so bad if a single wine process crashes.
That being said, if a Windows program works fine on Windows, then it should work fine in Wine, even if it does something stupid such as passing a NULL pointer to an API when it shouldn't. But for internal programming errors, crashing can be a good debug method: - it sure gets one's attention - it's supposed to generate a nice stack trace which you can use to complement the relay trace
Of course, that implies that we should promptly fix any such crash. Otherwise it just makes Wine unusable without any benefit.
On December 9, 2002 05:29 pm, vkxzjq6lg@cswebmail.com wrote:
Do you agree with the way I've explained how I see the difference between redundant defensive programming and detecting and reporting problems which need to be fixed? If not, where do you differ and why?
Let me try to explain a bit more what I mean.
There are two kinds of error conditions: 1. Programming Bugs: this include having inconsistent state, a NULL pointer when it should never be NULL, etc. 2. External Errors: other component failing, disk full, etc.
What I'm saying is that for (1) we should die (most of the time), and for (2) we should fail gracefully, and seldom signal the problem.
You see, this classification depends on categorizing the code into internal and external.
Internal code is what we control (such as the implementation of the a control, or a component, etc); in such internal code we get to decide the invariants, and it's OK (and preferable) that we crash if such an invariant doesn't hold true. I don't think it's acceptable to add tests all over the place that print error messages: it bloats the code, makes it slower, and uglifies it to the point where you can't figure out what you're actually trying to do. In such cases I consider such checks to be detrimental.
For external code, we no longer have the luxury to do what we want. Most (all?) Win32 APIs don't crash on NULL pointers, and so we should not crash either. Also, outputting a message in such a case is not needed -- this is normal behavior, and outputting messages just clutters the code, as well as the output. Again, such messages are detrimental. A few of the Win32 functions go a step further (oh, the horror!), and accept bad pointers without crashing. We should replicate such brain-dead behavior only if we know that (a) Windows does it this way in all versions, and (b) there are significant apps that depend on such behavior.
As you can see, there are few cases where outputting messages is waranted. It is mostly in places where we know we don't implement the entire specification. Too many messages are as bad as too few. We already output quite a few, adding tests and ERRs/WARNs all over the place is not a good idea.
-
Dimitrie O. Paun -
Francois Gouget -
vkxzjq6lg＠cswebmail.com