[pulseaudio-discuss] [PATCH 2/6] Turn device ports into reference counted objects

[David Henningsson]

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