[vlc-devel] [vlc-commits] strings: use vlc_memstream in vlc_xml_encode()
filip at atch.se
Sat Feb 25 09:49:28 CET 2017
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
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...
More information about the vlc-devel