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

[Filip Roséen]

Hi Rémi,

On 2017-02-25 15:27, Rémi Denis-Courmont wrote:

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

Just as practically impossible as memory-allocation failures.

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

You did not even reply to the section regarding that, and I still do
not see how the implementation is so non-trivial that it is a point
you bring up against error-checking.

> > > > > > `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 will leave these two links here for future readers:

 - https://mailman.videolan.org/pipermail/vlc-devel/2017-February/111711.html
 - https://mailman.videolan.org/pipermail/vlc-devel/2017-February/111713.html
> 
> >  - 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. 

That's not my opinion, my opinion (as stated numerous times) is that
the error-check is not invalid nor complicated enough to warrant it to
be non-existant.

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

You are certainly not the only one, and I am still interested in the
remarks you made regarding my contributions and the quality of what I
do (because I consider what you wrote very harsh, and not at all
inspiring in terms of future contribution).

As most often, I am available on IRC if you feel like clearifying.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170225/39fec1f2/attachment.html>