On 11/09/2011 01:15 AM, Colin Guthrie wrote:
'Twas brillig, and David Henningsson at 08/11/11 20:17 did gyre and gimble:
In my opinion assertions are proper error handling when the error in
question is a programming error in our own code.
Eh, I'd say proper error handling is to fix our own code's programming
error! :-)
I suspect what was meant was that when we accidentally call one of our
own functions incorrectly, then it should be good enough to hit an
assert to tell us we've hit the "error between the chair and the
keyboard" case. IMO, this is a valid use of asserts() to find and
eradicate this class of problem. Obviously it goes without saying that
the correct way to address this is to fix the calling code, but the
assert has done it's job well, so it's use is justified.
I'm not saying we should never use asserts, but I think we over-use
them. (And a lot of this goes back to Lennart's code as well.) Instead
of going the extra mile and thinking "hmm, what if this could actually
happen, what should we do in that case?", I suspect that sometimes we
just put in an assert instead, just out of laziness, and see if anyone
ever complains about it. True or not?
I think it's a fine line at times, but I think asserts() work well. What
I mean is that the laziness argument goes both ways and you can take the
opposite extreme that if you handle error cases more gracefully all you
get is a line in a log file for something that you really do want to
complain and or get upstream. Now an assert *is* brutal here, but if the
thing doesn't crash out on the user, we'll likely never know about the
issue and the underlying problem itself may go unnoticed.
So while it's arguably not user friendly at all times, I think it's a
good mechanism to ensure we have robust code over time. Yes, this means
we rely on downstreams cooperating and pushing bug reports (and
hopefully, as is often the case with your good self, bug fixes too) up
to us.
As code evolves, we'll commit new problems and asserts to PulseAudio.
Therefore we will never reach the "robust code" you're talking about in
practice, for PulseAudio as a whole. The result is PulseAudio getting
bad reputation.
But sure, from an upstream/developer perspective all these asserts make
perfect sense. For the end user, most of the time the "single log file
line" approach is better.
So; I think we've discussed the general case enough, back to where we
started:
I think this is cleaner and gives better error handling:
void foo_free(foo* f)
{
if (!f)
return;
/* Possibly more destruction stuff here */
pa_xfree(f);
}
/* Somewhere else */
foo_free(bar->f);
Than this:
void foo_free(foo* f)
{
pa_assert(f);
/* Possibly more destruction stuff here */
pa_xfree(f);
}
/* Somewhere else */
if (bar->f)
foo_free(bar->f);
The most common case for bar->f being null is that the bar object was
not completely initialized. In addition, in destructors you should never
throw exceptions.
If adjusting my code from the IMO better approach to the IMO worse
approach is what it takes to get the code in, I'll obviously do it. Let
me know if that is necessary.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
