<!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>On 2017-02-25 11:33, Rémi Denis-Courmont wrote:</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> But the VLC implemementation is not the only one, so that argument is
moot.</code></pre>
</blockquote>
<pre><code> No and I already explained why.</code></pre>
</blockquote>
<p>I do not agree with your explanation, so we can probably agree on disagreeing.</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> `vlc_memstream_open` can fail. If you consider it so unlikely
that one should not bother checking the *return-value* I do not see
why you put it there in the first place.</code></pre>
</blockquote>
<pre><code> This is a red herring.</code></pre>
</blockquote>
<p>It is very much relevant to the discussion and cuts straight to what you wrote about the unlikelihood of the function to fail (and not having to check it).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> I don´t want to take a bet on somebody actually having a good case to handle
the error in a specific future case or due to a future extension.</code></pre>
</blockquote>
<p>Wouldn’t that future extension be able to change the return-value back from <code>void</code> to <code>int</code>, if there then is a need for it? If I understand you correctly you state that there is no need for it at the current time, and that simply having it there can make implementations more complex as developers will be tempted to use it.</p>
<p>Removing a return-value can cause problems with refactoring, but adding one (for future use, in the future) generally does not (cause as much issues).</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">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Therefore, this will never be tested and never be run. There is no point
in optimizing for a case that never happens (insofar as it is otherwise
inconsequential). It only adds unnecessary complexity, platform dependent
behaviour, red lines in coverage testing, and eventually over time, bugs
in untested error paths.</code></pre>
</blockquote>
<pre><code> Then remove the return-value all together; it is contradicting to have
it with your expressed opinions.</code></pre>
</blockquote>
<pre><code> No, there is no contradiction. You are doing a false dichotomy where functions
have to return either void or a VLC_USED error code.</code></pre>
</blockquote>
<p>That is not what I am saying; and I have no idea how I am supposed to express myself in any other way than I already have.</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">
<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> For cases where having a premature error-check means a more
complicated implementation I do see a use for a "always ok"
`vlc_memstream_open`, but for trivial cases I'd rather see things be
checked - even though it is not mandatory.</code></pre>
</blockquote>
<pre><code> No. By that same argument, we would also check errors at each and every
vlc_memstream_*() write operation - and at least double the line count. I
wrote the implementation on purpose so that checking for errors was
entirely optional.</code></pre>
</blockquote>
<pre><code> It is trivial to check whether the initialization of the handle was
successful, it is however not trivial to have an if for every
write-operation (I have never claimed that I would recommend that, nor
have I done so when I have used the relevant functions).</code></pre>
</blockquote>
<pre><code> It is trivial there and now. What if one of those functions gets revectored
and handling the error is no longer a simple matter of a 'return' statement?
If you start adding the check everywhere that it is trivial just because you
can, this is what will happen next:
- Other developers will use your needlessly complicated code as example.
- They will assume that the error must be checked.
- Eventually, they will add unnecessary untested and non-trivial error
handling.
- Or worse, code will be revectored incorrectly, leading to bugs.</code></pre>
</blockquote>
<p>I have already stated my opinion.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> FWIW, the error check has already seemingly been needlessly copied by other
developers by example. And as a matter of facts, in two isntances (both
written by you) lead to non-trivial error handling.</code></pre>
</blockquote>
<p>We certainly have different ideas of what <em>trivial</em> and <em>non-trivial</em> means. For those who are not familiar with what parts you are referring to I took the liberty to link them below.</p>
<ul>
<li><p>http://git.videolan.org/?p=vlc.git;a=blob;f=modules/demux/ttml.c;h=d99653bb147a9c64506a7935125231202ed8f29a;hb=HEAD#l335</p></li>
<li><p>http://git.videolan.org/?p=vlc.git;a=blob;f=src/input/mrl_helpers.h;h=f4f815cef9857e7cf8aa999820f1a1fdbb2db410;hb=fcf559302e4#l70</p></li>
<li><p>http://git.videolan.org/?p=vlc.git;a=blob;f=modules/demux/ttml.c;h=d99653bb147a9c64506a7935125231202ed8f29a;hb=HEAD#l425</p></li>
</ul>
<p>The error-label would have been there in either case, it was not introduced because of the check related to <code>vlc_memstream_open</code>.</p>
<ul>
<li>http://git.videolan.org/?p=vlc.git;a=blob;f=src/input/stream_extractor.c;h=573ba9d19944bf523a6cccc68914f32755cd3d71;hb=fcf559302e4#l234</li>
</ul>
<p>No check whatsoever would of course have been more trivial, but I certainly do not see the implementation linked above as complicated related to <code>vlc_memstream_open</code>.</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> It surprises me to see your stance on this matter given that you have
yourself checked the *error-code* in `e9804d78697`; what was the
rationale in that case (if any)?</code></pre>
</blockquote>
<pre><code> First, questioning my own consistency is an ad hominem argument.</code></pre>
</blockquote>
<p>I did not question your consistency, I asked about the rationale in that particular case case since very strong opinions regarding why one should not do that was used. That should be considered on-topic, relevant, and a justifiable question.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> And then regardless, all of the three initial call sites, vlc_uri_compose()
and vlc_strfinput() _not_ check the result of vlc_memstream_open().</code></pre>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Literally, out of 8 call sites of vlc_memstream_open() that I wrote,
e9804d78697 contains the only site where I left the error check - because i
simply carried it over from the earlier malloc() check. I could, and in
hindsight, should have removed it too.</code></pre>
</blockquote>
<p>There is nothing for me to comment regarding the previous two sections.</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">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> The whole point of vlc_memstream is to _simplify_ formatting in memory.</code></pre>
</blockquote>
<pre><code> To check whether `vlc_memstream_open` is successful or not certainly
does not make the usage of `struct vlc_memstream` as complicated as
you seem to imply it does.</code></pre>
</blockquote>
<pre><code> You have shown a propensity to proposing debatable changes, framed as fixes
when they are just taste issues ("Missing check (...)"). This is annoying. You
can´t keep doing that and not expect a frustating time-wasting bikeshed
follow-up discussion. You can´t blame me for that when you trigger it.</code></pre>
</blockquote>
<p>First of all I do not see how this is relevant, and second; I would also like to know where in this thread I have blamed you for these discussions.</p>
<p>If, as you state, my patches result in <em>“time-wasting bikeshed”</em>, are poorly written, and are only *“framed” to be fixes (implying that they do more harm than good); I will be the first to step back and not submit any more patches.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> And sorry but you can´t expect me or anybody else to be 100% self-consistent.
Every human here is fallible, particularly me.</code></pre>
</blockquote>
<p>I do not expect such; not sure where that is coming from, nor why you feel like it has to be stated.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> With that said, I should note that people point out errors, propose or make
fixes to code that I wrote often and that is of course totally fine. Thomas
did it no later than yesterday.</code></pre>
</blockquote>
</body>
</html>