[vlc-devel] [PATCH 1/8] stream_extractor: split joint capability into two
filip at atch.se
Sat Feb 18 10:30:40 CET 2017
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
> > + /**
> > + * 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.
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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the vlc-devel