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

[Rémi Denis-Courmont]

Le lauantaina 25. helmikuuta 2017, 13.15.47 EET Filip Roséen a écrit :
> Hi Rémi,
> 
> I am sad to see that you left out parts of my reply, especially those
> regarding my contributions and how they result in a waste of time, how
> my patches are "framed" (which I still do not know what you mean by),
> 
> On 2017-02-25 13:30, Rémi Denis-Courmont wrote:
> > Le lauantaina 25. helmikuuta 2017, 11.13.22 EET Filip Roséen a écrit :
> > > 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.
> > 
> > The explanation is factual. One does not get to agree or disagree with it.
> 
> Ok, now I completely am out of words. I thought this was a discussion,
> not a place where "factual" statements expressed by one party are the
> only truth, and opinions of the other party does not matter.

- It is a fact, not an opinion, that ignoring an error here is safe.
- It is a fact, not an opinion, that an error cannot happen with one back-end.
- It is a fact, not an opinion, that an error is practically impossible to 
trigger with the other back-end.

It is therefore also a fact that adding the error handling there will:
- make the source code marginally more complex,
- make the binary code marginally slower,
- make code coverage worse.

And it is an extremely likely conjecture if not a proven fact that the error 
handling:
- will not be tested much if at all,
- will become a (good or bad) example for future code.

Also the commit message is misleading since the error check while absent is 
not "missing". And the code might be revectored or copied in circumstances 
that would require less trivial and more bug-prone handling. That too would 
normally be a conjecture rather than a fact, except that this has already 
occurred and been proven true in other code paths (TTML, MRL helpers).

> > > > > `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).
> > 
> > You are again making the false dichotomy which you yourself deny.
> > There is an error return because there is a possible error case.
> > There is no "missing check" (and no VLC_USED qualifier) because the
> > error can be safely ignored.
> 
> I think I have explained my ground in previous posts, but to
> summarize:
> 
>  - I do not see why it is so bad introducing checks for the
>    *return-value* (because I do not agree with your statement
>    regarding how non-trivial an implemenation becomes), and;

I never said it was "so bad". Indeed, it does not add any immediate bug. I do 
believe that it makes the code worse rather than better, and thus am rejecting 
the patch.

>  - I certainly do not see why removing the *return-value* would be
>    foolish (when actually handling it is, as stated, wrong in most
>    (all) cases).

If an error is possible, it looks perfectly normal and reasonable to return an 
error status, even if the error can safely be ignored by the caller. I do not 
see why that specific function would be any different from the myriad of 
functions that follow the same pattern (in VLC and elsewhere).

You seem to imply that error handling should either be mandatory or forbidden. 
But I did purposedly leave error handling *optional* in this case, because it 
added no practical cost on the implementation(*) on the one hand, and gave 
flexibility to the user on the other hand.

And FWIW, it is Thomas, not I, who decided to make use the second option by 
handling the error. All three original use cases ignored it.


And I am very very very fed up with this discussion so that will be my last 
mail.

-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/