On 4/24/2017 11:55 AM, Vittorio Giovara wrote:
> On Sun, Apr 23, 2017 at 11:31 PM, James Almer <jamr...@gmail.com> wrote:
>> On 4/20/2017 12:34 PM, Vittorio Giovara wrote:
>>>
>>> Change input type of the type->str function and return a proper
>>> error code for the str->type function.
>>> ---
>>> Similar reasoning to the previous patch.
>>> Vittorio
>>>
>>> libavutil/stereo3d.c | 6 +++---
>>> libavutil/stereo3d.h | 2 +-
>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavutil/stereo3d.c b/libavutil/stereo3d.c
>>> index 5dc902e909..6e2f9f3ad2 100644
>>> --- a/libavutil/stereo3d.c
>>> +++ b/libavutil/stereo3d.c
>>> @@ -54,9 +54,9 @@ static const char * const stereo3d_type_names[] = {
>>> [AV_STEREO3D_COLUMNS] = "interleaved columns",
>>> };
>>> -const char *av_stereo3d_type_name(unsigned int type)
>>> +const char *av_stereo3d_type_name(enum AVStereo3DType type)
>>> {
>>> - if (type >= FF_ARRAY_ELEMS(stereo3d_type_names))
>>> + if ((unsigned) type >= FF_ARRAY_ELEMS(stereo3d_type_names))
>>> return "unknown";
>>> return stereo3d_type_names[type];
>>> @@ -72,5 +72,5 @@ int av_stereo3d_from_name(const char *name)
>>> return i;
>>> }
>>> - return -1;
>>> + return AVERROR(EINVAL);
>>
>>
>> Why EINVAL here but ENOSYS for spherical?
>
> no reason, i can switch to enonsys
I was thinking EINVAL is more correct for both cases.
>
>> Also, while changing -1 to AVERROR() isn't a considerable API break as
>> changing NULL on failure to a string on failure, it's still one and really
>> makes me wonder if it's worth doing.
>> While most users probably just check for < 0, in which case both return
>> values would work the same, anyone instead checking explicitly for -1 (a
>> completely valid check as per the documentation, which you for that matter
>> did not update in this patch) will instead break.
>
> besides the fact that we're in the "break period" after the version
> bump, there should be no reason to explicitly check for -1 so I don't
> think that's a case that it's worth considering
>
As Anton already stated, there is no such thing as an API breaking
season, only ABI breaking season. You can't change a function signature
or return value/type on a whim if it can break downstream code.
The documentation states the function returns -1 on failure. Explicitly
checking for said value is a correct and completely valid way to check
for failure. This patch would break such downstream code that follows
the documentation to the letter.
Changing unsigned int to enum AVStereo3DType as parameter type in
av_stereo3d_type_name() however should be ok. No API user will be
affected by that.
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
