[FFmpeg-devel] [PATCH 1/2] lavu/opt: fix av_opt_get_key_value() API.

[Stefano Sabatini]

On date Thursday 2012-11-15 12:10:30 +0100, Nicolas George encoded:
> Le quintidi 25 brumaire, an CCXXI, Stefano Sabatini a écrit :
> > Reading the current docs I realize it is not very clear. What I'd
> > expect:
> > 
> > [foo=bar:baz=qux]
> >  ^
> > 
> > after parsing:
> > [foo=bar:baz=qux]
> >         ^
> >         ropts
> > 
> > second call (after updating ropts):
> > [foo=bar:baz=qux]
> >                 ^
> >                 ropts
> 
> When doing the bulk of the parsing, having ropts point to baz instead of the
> : is more practical. I implemented it that way initially.
> 
> > According to docs:
> > |Extract a key-value pair from the beginning of a string.
> > so I'd assume this "consumes" the parsed input. This is not always
> > very practical but I'd assume it is the most natural expected
> > behavior for a parsing function.
> 
> Yes, it consumes the parsed input, including the delimiter.
> 
> > Here "rest of string" is ambiguous.
> 
> I'll make it more specific.
> 
> > While at it, I also suggest to change:
> > |@return  0 for success, or a negative value corresponding to an AVERROR
> > |           code in case of error; in particular:
> > |           AVERROR(EINVAL) if no key is present
> > in order to return >= 0 in case of success, in case we want to provide
> > more information.
> 
> Ok, not a problem.
> 
> > For example: it might return 0 in case of empty string (so not
> > considered like an error), or the number of consumed chars, which
> > might turn useful when iterating.
> 
> Yes, that may be useful.
> 
> > If you want my opinion, I'd rather change the way ropt is updated, and
> > add the check if (*ropts && strchr(delims, *ropts)) *ropts++; in the
> > surrounding code, even if a little annoying looks more predictable.
> 

> I find that construct much less practical and much more error-prone (should
> be using strchr? strspn? strcspn? when you are familiar with the code there
> is no ambiguity, but that is asking a lot) than returning the delimiter in a
> separate argument.

I believe that not consuming the delimiter is the most natural and
expected behavior (and is followed by most part of the parsing code in
FFmpeg). It also delivers more control over the parsing, which usually
means that the user will have to use more code to get around the
task. In particular it avoids the need for the additional argument or
for clumsy backward parsing.
-- 
FFmpeg = Friendly and Fundamentalist Mythic Perfectionist Experimenting Geisha