[vlc-devel] [PATCH] core: Add atomic refcounter helper

[Rémi Denis-Courmont]

Le samedi 30 juin 2018, 23:49:23 EEST Romain Vimont a écrit :
> On Sat, Jun 30, 2018 at 10:28:14PM +0200, Rémi Denis-Courmont wrote:
> > The only times I recall that somebody screwed up reference counting where
> > nontrivial cases not handled by this patch.
> In network/httpd.c, httpd_HostDelete():
> 
>     if (atomic_fetch_sub_explicit(&host->ref, 1, memory_order_relaxed) > 1)
> 
> I'm not sure a memory_order_relaxed is sufficient here, for the reasons
> explained in the video (the data need to be seen by the thread that
> destroys the object).

I fail to see what a video on reference counting would have to do with this.

> > And the Boolean result is not obvious at all. As far as error proneness
> > comes, this is actually *worse*.
> That's an opinion, I don't share it.

No it is not an opinion. It is a count. This patch contains two already 
mentioned errors, three with the above paragraph. That is probably more than 
the current approach has ever had in many years. Three is more than zero, and 
that makes this patch more prone to errors based on circumstancial evidence.

Furthermore the boolean evaluation of the return value is the opposite of what 
any other ref counting API does (other than VLC's). Usually, zero is for last 
reference and non-zero for non-last references. Again, this is prone to 
errors.

And to top all of that, this does not even address the actually tricky cases 
where programmers are actually prone to making non-obvious mistakes.

So no thanks.

-- 
Rémi Denis-Courmont