[vlc-devel] [vlc-commits] strings: use vlc_memstream in vlc_xml_encode()
remi at remlab.net
Sat Feb 25 10:33:05 CET 2017
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
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
- 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.
More information about the vlc-devel