<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Rémi,</p>
<p>Thank you for your comments.</p>
<p>On 2017-02-18 10:06, Rémi Denis-Courmont wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Le torstaina 16. helmikuuta 2017, 22.44.11 EET Filip Roséen a écrit :</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + * The following members shall be populated as specified by the
+ * documentation associated with \stream_t for the equivalent name.</code></pre>
</blockquote>
<pre><code> Does this actually work? It looks weird; I´d expect \ref stream_t or such...</code></pre>
</blockquote>
<p>The backslash is actually an unfortunate typo, <em>doxygen</em> will automatically create a link from “stream_t” on it’s own as it recognize that this is a type.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + char const* identifier; /**< the name of the entity to be extracted */</code></pre>
</blockquote>
<pre><code> This is valid but I don´t know why a very few developpers do this. The defacto
standard is qualifier before type.</code></pre>
</blockquote>
<p>Not sure what <em>“defacto standard”</em> 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 <em>“read types right-to-left”</em> idiom.</p>
<p>For consistancy, I much prefer <code>char const * const</code> vs <code>const char * const</code>; though I do not mind developers writing it differently, especially not in a <em>translation-unit</em> on its own.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> typedef struct stream_extractor_t stream_extractor_t;
+typedef struct stream_directory_t stream_directory_t;</code></pre>
</blockquote>
<pre><code> Should be merged with the struct typedef (when possible).</code></pre>
</blockquote>
<p>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).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> /**
* 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</code></pre>
</blockquote>
<pre><code> The (...).</code></pre>
</blockquote>
<p>I am unable to figure out what <em>“(…)”</em> is referring to; could you elaborate?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + /**
+ * 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* );</code></pre>
</blockquote>
<pre><code> I don´t really get why this is needed inside a private structure.
[...]
Ditto.</code></pre>
</blockquote>
<p>Because the <em>initialization</em> and <em>clean-up</em> is private.</p>
<p>If you are really asking why <code>pf_init</code> and <code>pf_clean</code> is used instead of simply branching at the places depending on some flag within <code>struct stream_extractor_private</code>, or <code>vlc_object_t->obj.object_type</code>; that is certainly a possibility (but it was discarded when coming up with the design).</p>
<p>For one I find such branching to hinder readability.</p>
<p>Best Regards,<br />
Filip</p>
</body>
</html>