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

[Nicolas George]

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.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121115/cab7f155/attachment.asc>