Re: [libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[Vittorio Giovara]

On Mon, Apr 24, 2017 at 11:24 AM, James Almer  wrote:
> On 4/24/2017 11:55 AM, Vittorio Giovara wrote:
>> On Sun, Apr 23, 2017 at 11:31 PM, James Almer  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

Re: [libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[James Almer]

On 4/24/2017 11:55 AM, Vittorio Giovara wrote:
> On Sun, Apr 23, 2017 at 11:31 PM, James Almer  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

Re: [libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[Vittorio Giovara]

On Sun, Apr 23, 2017 at 11:31 PM, James Almer  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

> 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
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[James Almer]

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?

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.



  }
diff --git a/libavutil/stereo3d.h b/libavutil/stereo3d.h
index 0fa9f63a2c..cbf138faef 100644
--- a/libavutil/stereo3d.h
+++ b/libavutil/stereo3d.h
@@ -190,7 +190,7 @@ AVStereo3D *av_stereo3d_create_side_data(AVFrame *frame);
   *
   * @return The name of the stereo3d value, or "unknown".
   */
-const char *av_stereo3d_type_name(unsigned int type);
+const char *av_stereo3d_type_name(enum AVStereo3DType type);
  
  /**

   * Get the AVStereo3DType form a human-readable name.



___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[Luca Barbato]

On 4/20/17 5: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
> 

Fine for me.

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [RFC] [PATCH] stereo3d: Update the naming API to be more consistent

[Vittorio Giovara]

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);
 }
diff --git a/libavutil/stereo3d.h b/libavutil/stereo3d.h
index 0fa9f63a2c..cbf138faef 100644
--- a/libavutil/stereo3d.h
+++ b/libavutil/stereo3d.h
@@ -190,7 +190,7 @@ AVStereo3D *av_stereo3d_create_side_data(AVFrame *frame);
  *
  * @return The name of the stereo3d value, or "unknown".
  */
-const char *av_stereo3d_type_name(unsigned int type);
+const char *av_stereo3d_type_name(enum AVStereo3DType type);
 
 /**
  * Get the AVStereo3DType form a human-readable name.
-- 
2.12.0

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel