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

[Filip Roséen]

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

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

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

> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170225/596ea1fb/attachment.html>