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

[Colin Guthrie]

'Twas brillig, and David Henningsson at 09/11/11 09:42 did gyre and gimble:
> 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.


I could fairly easily be persuaded to agree that for "free" functions as
you listed above, asserting is likely overkill. After all pa_xfree(NULL)
is quite happy, so why should any higher level free function complain
more bitterly?

But free functions are arguably a particular class of function. Not
calling free properly only (usually) results in memory leaks.

I certainly wouldn't like a module to call pa_sink_input_move_to() with
invalid pointers and have that handled gracefully. I'd like the
conditions that lead up to the incorrect call to be very much reported
in a backtrace should it ever get out in a release.

Otherwise what happens? The stream is not moved and a something gets
written in a log and some key functionality just doesn't work but often
the user will be none the wise... "Hmm, I thought plugging in my USB was
meant to switch my music to it but it didn't... ho hum never mine"...
just an anonymous entry in a log and nothing gets reported etc. etc.

So for this class of problem, that is clear programming error - using
the APIs wrong - I'm still very much in favour of the assert usage.

But that's just my opinion, and I know it's certainly far from universal.

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/