[vlc-devel] [vlc-commits] strings: use vlc_memstream in vlc_xml_encode()

[Filip Roséen]

Hi Rémi,

On 2017-02-25 11:33, Rémi Denis-Courmont wrote:

> > But the VLC implemementation is not the only one, so that argument is
> > moot.
> 
> No and I already explained why.

I do not agree with your explanation, so we can probably agree on
disagreeing.

> > `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.
> 
> This is a red herring.

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).
 
> 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.

Wouldn't that future extension be able to change the return-value back
from `void` to `int`, 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.

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).


> > > 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.
> > 
> > Then remove the return-value all together; it is contradicting to have
> > it with your expressed opinions.
> 
> No, there is no contradiction. You are doing a false dichotomy where functions 
> have to return either void or a VLC_USED error code.

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.

> > > > 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.
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

I have already stated my opinion.

> 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.

We certainly have different ideas of what *trivial* and *non-trivial*
means. For those who are not familiar with what parts you are
referring to I took the liberty to link them below.

 - http://git.videolan.org/?p=vlc.git;a=blob;f=modules/demux/ttml.c;h=d99653bb147a9c64506a7935125231202ed8f29a;hb=HEAD#l335

 - http://git.videolan.org/?p=vlc.git;a=blob;f=src/input/mrl_helpers.h;h=f4f815cef9857e7cf8aa999820f1a1fdbb2db410;hb=fcf559302e4#l70

 - http://git.videolan.org/?p=vlc.git;a=blob;f=modules/demux/ttml.c;h=d99653bb147a9c64506a7935125231202ed8f29a;hb=HEAD#l425

   The error-label would have been there in either case, it was not
   introduced because of the check related to `vlc_memstream_open`.

 - http://git.videolan.org/?p=vlc.git;a=blob;f=src/input/stream_extractor.c;h=573ba9d19944bf523a6cccc68914f32755cd3d71;hb=fcf559302e4#l234

   No check whatsoever would of course have been more trivial, but I
   certainly do not see the implementation linked above as complicated
   related to `vlc_memstream_open`.

> > 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)?
> 
> First, questioning my own consistency is an ad hominem argument.

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.
 
> And then regardless, all of the three initial call sites, vlc_uri_compose() 
> and vlc_strfinput()  _not_ check the result of vlc_memstream_open().

> 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.

There is nothing for me to comment regarding the previous two
sections.

> > > The whole point of vlc_memstream is to _simplify_ formatting in memory.
> > 
> > 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.
> 
> 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.

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.

If, as you state, my patches result in *"time-wasting bikeshed"*, 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.

> And sorry but you can´t expect me or anybody else to be 100% self-consistent. 
> Every human here is fallible, particularly me.

I do not expect such; not sure where that is coming from, nor why you
feel like it has to be stated.

> 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.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170225/c6ee13f6/attachment.html>