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

[Rémi Denis-Courmont]

Le lauantaina 25. helmikuuta 2017, 9.49.28 EET Filip Roséen a écrit :
> Hi Rémi,
> 
> On 2017-02-25 10:36, Rémi Denis-Courmont wrote:
> > Le lauantaina 25. helmikuuta 2017, 9.22.30 EET Filip Roséen a écrit :
> > > Hi Rémi,
> > > 
> > > On 2017-02-25 10:11, Rémi Denis-Courmont wrote:
> > > > > Missing check to see if `vlc_memstream_open` actually created a
> > > > > valid
> > > > > handle.
> > > > 
> > > > No. It is, by design, safe to ignore vlc_memstream_open() failure.
> > > > Otherwise, I´d have marked it VLC_USED.
> > > 
> > > I know that it is safe, but I do not see any reason to ignore the
> > > *error-code*.
> > 
> > This error:
> > - cannot happen if the VLC implementation is used,
> > - is nigh-impossible to trigger with the native implementation.
> 
> But the VLC implemementation is not the only one, so that argument is
> moot.

No and I already explained why.

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

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.

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

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

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.

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

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.

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

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

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.

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