[vlc-devel] [PATCH 1/8] stream_extractor: split joint capability into two

[Filip Roséen]

Hi Rémi,

Thank you for your comments.

On 2017-02-18 10:06, Rémi Denis-Courmont wrote:

> Le torstaina 16. helmikuuta 2017, 22.44.11 EET Filip Roséen a écrit :
> > +     * The following members shall be populated as specified by the
> > +     * documentation associated with \stream_t for the equivalent name.
> 
> Does this actually work? It looks weird; I´d expect \ref stream_t or such...

The backslash is actually an unfortunate typo, *doxygen* will
automatically create a link from "stream_t" on it's own as it
recognize that this is a type.

> > +    char const* identifier; /**< the name of the entity to be extracted */
> 
> This is valid but I don´t know why a very few developpers do this. The defacto 
> standard is qualifier before type.

Not sure what *"defacto standard"* you are referring to, but I know
many developers who always put cv-qualifiers to the right of what they
refer to for consistancy reasons, while further emphasizing the *"read
types right-to-left"* idiom.

For consistancy, I much prefer `char const * const` vs `const char *
const`; though I do not mind developers writing it differently,
especially not in a *translation-unit* on its own.

> >  typedef struct stream_extractor_t stream_extractor_t;
> > +typedef struct stream_directory_t stream_directory_t;
> 
> Should be merged with the struct typedef (when possible).

We should spend some time writing up a coding guideline so that we do
not have to end up with reviews such as this (where it is quite
obvious that things boil down to a personal taste).

> > 
> >  /**
> >   * Create a relative MRL for the associated entity
> >   *
> > - * This function shall be used by stream_extractor_t's in order to
> > - * generate a MRL that refers to an entity within the stream. Normally
> > + * This function shall be used by stream_directory_t's in order to
> > + * generate an MRL that refers to an entity within the stream. Normally
> >   * this function will only be invoked within `pf_readdir` in order to
> >   * get the virtual path of the listed items.
> >   *
> >   * \warning the returned value is to be freed by the caller
> 
> The (...).

I am unable to figure out what *"(...)"* is referring to; could you
elaborate?

> > +    /**
> > +     * Callback to handle initialization
> > +     *
> > +     * \pf_init will be called after successful module probing to
> > initialize +     * the relevant members of the underlying stream-extractor
> > object, as well +     * as the wrapping stream.
> > +     **/
> > +    int (*pf_init)( struct stream_extractor_private*, stream_t* );
> 
> I don´t really get why this is needed inside a private structure.
> [...]
> Ditto.

Because the *initialization* and *clean-up* is private.

If you are really asking why `pf_init` and `pf_clean` is used instead
of simply branching at the places depending on some flag within
`struct stream_extractor_private`, or `vlc_object_t->obj.object_type`;
that is certainly a possibility (but it was discarded when coming up
with the design).

For one I find such branching to hinder readability.

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