[vlc-devel] [PATCH 3/6] vlc_stream: add vlc_stream_NewMRL

[Filip Roséen]

Hi Rémi,

Thanks for the very fast feedback, as always much appreciated!

On 2017-03-28 20:56, Rémi Denis-Courmont wrote:

> Le perjantaina 17. maaliskuuta 2017, 3.22.33 EEST Filip Roséen a écrit :
> > ---
> >  include/vlc_stream.h         | 16 ++++++++++++++++
> >  src/input/stream.c           | 27 +++++++++++++++++++++++++++
> >  src/input/stream_extractor.c |  6 ------
> >  src/libvlccore.sym           |  1 +
> >  4 files changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/vlc_stream.h b/include/vlc_stream.h
> > index 3a6d6ccd0e..55350d6b30 100644
> > --- a/include/vlc_stream.h
> > +++ b/include/vlc_stream.h
> > @@ -350,6 +350,22 @@ VLC_USED;
> >  #define vlc_stream_NewURL(a, b) vlc_stream_NewURL(VLC_OBJECT(a), b)
> > 
> >  /**
> > + * Create a stream_t reading from a \ref MRL.
> > + * You must delete it using vlc_stream_Delete.
> > + *
> > + * \warning This function shall only be used when MRL functionality is
> > + *          explicitly needed (such as when referring to items within an
> > + *          archive). \ref vlc_stream_NewURL shall be used where
> > applicable.
> 
> Frankly, *I* did not understand what that distinction meant until I read the 
> code below. Now, I can see the logic, and I don´t think that my own 
> documentation is great either. But I find this text very unhelpful.

Yes, I was myself struggling quite a bit to come up with a wording
that didn't span too many lines that actually reflected what the
distinction is.

I was hoping that the reference to the MRL-documentation would be a
good way of signifying the difference, but I will try to come up with
a better wording (and might try it out on a few non-VLC developers
just to see if it works).
 
> > + *
> > + * \param obj the parent of the requested stream
> > + * \param mrl the mrl for which the stream_t should be created
> > + * \return NULL on error, pointer to stream_t on success
> > + */
> > +VLC_API stream_t * vlc_stream_NewMRL(vlc_object_t *obj, const char *mrl)
> > +VLC_USED;
> > +#define vlc_stream_NewMRL(a, b) vlc_stream_NewMRL(VLC_OBJECT(a), b)
> 
> And regardless of how much you manage to improve the documentation, if you 
> leave this function in vlc_stream.h, people will (mis)use it, because they 
> cultivate cargo instead of reading the documentation.
> 
> It´s not your fault, but I think you should put it in the extractor header.

That seems like an excellent idea, though the reason I put it in
`include/vlc_stream.h` was due to the return-type as well as the
functions name.

But the more I think about it, the more I think you are correct; I
will move it to `include/vlc_stream_extractor.h` as it is tightly
coupled with *stream-extractors* in either case.

Best Regards,
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170328/fb61aef5/attachment.html>