Hello Jörg,
as always it depends.
On 09/27/2012 09:37 AM, Joerg-Cyril.Hoehle@t-systems.com wrote:
+#include <assert.h>
your patch is already committed, yet I believe that the least that dsound needs is new asserts.
Asserts in dsound have been a PITA in Wine for the last decade.
It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound.
The assert's backtrace will not help anybody, because the logical error will have happened much earlier.
I disagree. It helps GyB to open regression bug reports and it helps me to fix them.
Instead, a plain ERR would have been just as telling. Let's quote http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES "ERR: Messages in this class indicate serious errors in Wine, such as as conditions that should never happen by design."
I recommend:
- assert(device->buffers[0] == pDSB);
- if (pdevice->buffers[0] != pDSB)
ERR("broken device buffer %p\n", device->buffers[0]); HeapFree(GetProcessHeap(), 0, device->buffers); device->buffers = NULL;The ERR is IMHO revealing enough in a user's log.
That's too late if it hits the user.
I had reviewed the code: That code is an implementation detail completely outside the control of the applications. Barring a memory corruption there is no way for it to fail. The assert is there for two reasons: - To crash during "make test" when the next developer touches that code. - As a comment less likely to be removed by Alexandre ;)
If a sound component detects a bogus state, I'd very much prefer that it disables sound -- without crashing.
In general "it depends" of course.
In this particular case the ERR would be useless noise as the assert is only for the last sound buffer that is removed; usually at program exit. Until then the code does what you prefer: silently drops sound buffers without crashing thus deferring the crash to the program exit. A crash on program exit does get reported, more so than an ERR on the command line.
bye michael