[vlc-devel] [PATCH] lib: assert instead of abort

[Alexandre Janniaux]

Hi, this is fine with me too. This is not a failure (like xfuncs) but a
developer error so assert seems fine to me.

However, this shows that this API is a bit clunky from the user point of
view, maybe we could trac a ticket to move to the same idea as
vlc_player_listener or vlc_playlist_listener and move the check to the
API user land, then just keep an assert != NULL at the top of the function
instead ?

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Sep 27, 2019 at 11:22:34AM +0200, Thomas Guillem wrote:
> So, can I push this patch ? I think that Romain have very good arguments.
>
> On Fri, Sep 20, 2019, at 14:49, Romain Vimont wrote:
> > On Fri, Sep 20, 2019 at 03:13:08PM +0300, Rémi Denis-Courmont wrote:
> > > I completely disagree. When event registration/deregistration fails, you're almost guaranteed a deadlock or some use after free. Much better to abort asap.
> >
> > If an assertion fails, then the program has (probably) undefined
> > behavior.
> >
> > But why using assert() everywhere but abort() here? There are a lot of
> > places where a failing assert() would almost guarantee a deadlock or
> > some use after free.
> >
> > > Le 20 septembre 2019 12:21:37 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
> > > >Because you should not abort in production. Developers will still be
> > > >notified
> > > >that they are miss-using the API with the assert.
> > > >
> > > >PS: this abort was happening on iOS via VLCKit, the API miss-use had
> > > >been fixed
> > > >here: https://code.videolan.org/videolan/VLCKit/merge_requests/19
> > > >---
> > > > lib/event.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > >diff --git a/lib/event.c b/lib/event.c
> > > >index 28d3d0c232..4a57029b89 100644
> > > >--- a/lib/event.c
> > > >+++ b/lib/event.c
> > > >@@ -179,5 +179,5 @@ void libvlc_event_detach(libvlc_event_manager_t
> > > >*em, libvlc_event_type_t type,
> > > >              return;
> > > >          }
> > > >     }
> > > >-    abort();
> > > >+    assert(!"event not found or already detached");
> > > > }
> > > >--
> > > >2.20.1
> > > >
> > > >_______________________________________________
> > > >vlc-devel mailing list
> > > >To unsubscribe or modify your subscription options:
> > > >https://mailman.videolan.org/listinfo/vlc-devel
> > >
> > > --
> > > Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel