On Mon, Apr 24, 2017 at 11:24 AM, James Almer <jamr...@gmail.com> wrote:
> 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.
That's not following documentation to the letter, is writing code that
it is not future proof.
In 99% of places using "ret < 0" is the valid and recommended way of
checking for errors, unless you need to validate against explicit and
documented values. A -1 is a terrible return value, and I'd rather
break the 1 or 2 theoretical uses (during the period where people need
to review their code anyway if they wish to continue using a recent
library) than propagating this wrong practice more.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
