[FFmpeg-devel] avutils/opt: fix discarded-qualifiers compiler warning

---
 libavutil/opt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Nuo Mi:
> ---
>  libavutil/opt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 590146b5fb..c554e9c063 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const AVClass *parent, void **iter)
>  #if FF_API_CHILD_CLASS_NEXT
>  FF_DISABLE_DEPRECATION_WARNINGS
>      if (parent->child_class_next) {
> -        *iter = parent->child_class_next(*iter);
> +        *iter = (void*)parent->child_class_next(*iter);
>          return *iter;
>      }
>  FF_ENABLE_DEPRECATION_WARNINGS
> 
This doesn't look like a fix; you just silenced a warning.

- Andreas

On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Nuo Mi:
> > ---
> >  libavutil/opt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > index 590146b5fb..c554e9c063 100644
> > --- a/libavutil/opt.c
> > +++ b/libavutil/opt.c
> > @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
> AVClass *parent, void **iter)
> >  #if FF_API_CHILD_CLASS_NEXT
> >  FF_DISABLE_DEPRECATION_WARNINGS
> >      if (parent->child_class_next) {
> > -        *iter = parent->child_class_next(*iter);
> > +        *iter = (void*)parent->child_class_next(*iter);
> >          return *iter;
> >      }
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >
> This doesn't look like a fix; you just silenced a warning.
>
Yes, but this the only way if we do not change the function signature.

>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Nuo Mi:
> On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Nuo Mi:
>>> ---
>>>  libavutil/opt.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index 590146b5fb..c554e9c063 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
>> AVClass *parent, void **iter)
>>>  #if FF_API_CHILD_CLASS_NEXT
>>>  FF_DISABLE_DEPRECATION_WARNINGS
>>>      if (parent->child_class_next) {
>>> -        *iter = parent->child_class_next(*iter);
>>> +        *iter = (void*)parent->child_class_next(*iter);
>>>          return *iter;
>>>      }
>>>  FF_ENABLE_DEPRECATION_WARNINGS
>>>
>> This doesn't look like a fix; you just silenced a warning.
>>
> Yes, but this the only way if we do not change the function signature.
> 
There is another way: Do nothing. The code in question is deprecated and
will therefore eventually be removed.

- Andreas

On 2/14/2021 10:16 AM, Andreas Rheinhardt wrote:
> Nuo Mi:
>> On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
>> andreas.rheinhardt@gmail.com> wrote:
>>
>>> Nuo Mi:
>>>> ---
>>>>   libavutil/opt.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>>> index 590146b5fb..c554e9c063 100644
>>>> --- a/libavutil/opt.c
>>>> +++ b/libavutil/opt.c
>>>> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
>>> AVClass *parent, void **iter)
>>>>   #if FF_API_CHILD_CLASS_NEXT
>>>>   FF_DISABLE_DEPRECATION_WARNINGS
>>>>       if (parent->child_class_next) {
>>>> -        *iter = parent->child_class_next(*iter);
>>>> +        *iter = (void*)parent->child_class_next(*iter);
>>>>           return *iter;
>>>>       }
>>>>   FF_ENABLE_DEPRECATION_WARNINGS
>>>>
>>> This doesn't look like a fix; you just silenced a warning.
>>>
>> Yes, but this the only way if we do not change the function signature.

Suppressing a warning that something isn't right (i.e. that maybe the 
function signature *should* be changed) probably isn't ideal, except as 
a last resort. The fact that the documentation literally says to do the 
cast this patch is trying to do only compounds the problem.

I can understand wanting to use 'void *' for the context, in order to 
hide the underlying implementation. But since the documentation has 
already implied an implementation, hiding it may be moot. However, in 
any implementation, it really should be 'const void **', as iteration 
should have no internal side effects.

This leads back to the original statement of not covering up the problem 
(i.e. no nothing now -- but not just because it's deprecated) and 
encourage a signature change to be done (at an appropriate API breakage 
point).


> There is another way: Do nothing. The code in question is deprecated and
> will therefore eventually be removed.

On 2/14/2021 10:12 AM, Nuo Mi wrote:
> On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> Nuo Mi:
>>> ---
>>>   libavutil/opt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index 590146b5fb..c554e9c063 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
>> AVClass *parent, void **iter)
>>>   #if FF_API_CHILD_CLASS_NEXT
>>>   FF_DISABLE_DEPRECATION_WARNINGS
>>>       if (parent->child_class_next) {
>>> -        *iter = parent->child_class_next(*iter);
>>> +        *iter = (void*)parent->child_class_next(*iter);
>>>           return *iter;
>>>       }
>>>   FF_ENABLE_DEPRECATION_WARNINGS
>>>
>> This doesn't look like a fix; you just silenced a warning.
>>
> Yes, but this the only way if we do not change the function signature.

Since a major release is "coming up", now could be the time to fix the 
signature to '(..., const void **iter)' and eliminate the non-const cast 
kludge across the board for these cases.


-Chad

On Tue, Mar 9, 2021 at 3:17 AM Chad Fraleigh <chadf@triularity.org> wrote:

> On 2/14/2021 10:12 AM, Nuo Mi wrote:
> > On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
> > andreas.rheinhardt@gmail.com> wrote:
> >
> >> Nuo Mi:
> >>> ---
> >>>   libavutil/opt.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c
> >>> index 590146b5fb..c554e9c063 100644
> >>> --- a/libavutil/opt.c
> >>> +++ b/libavutil/opt.c
> >>> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
> >> AVClass *parent, void **iter)
> >>>   #if FF_API_CHILD_CLASS_NEXT
> >>>   FF_DISABLE_DEPRECATION_WARNINGS
> >>>       if (parent->child_class_next) {
> >>> -        *iter = parent->child_class_next(*iter);
> >>> +        *iter = (void*)parent->child_class_next(*iter);
> >>>           return *iter;
> >>>       }
> >>>   FF_ENABLE_DEPRECATION_WARNINGS
> >>>
> >> This doesn't look like a fix; you just silenced a warning.
> >>
> > Yes, but this the only way if we do not change the function signature.
>
> Since a major release is "coming up", now could be the time to fix the
> signature to '(..., const void **iter)' and eliminate the non-const cast
> kludge across the board for these cases.

Are we plan to remove all warnings before the release?
Or more aggressively, is it feasible to treat all warnings as errors in the
future? :)

>


>
> -Chad
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Nuo Mi (12021-03-09):
> Or more aggressively, is it feasible to treat all warnings as errors in the
> future? :)

No. Warnings come and go, and a lot of new warnings are bogus.

For example the new warning that tells you that snprintf(buf,
sizeof(buf), "%d", x) will overflow if sizeof(buf) < 11 even though we
know that x is between 0 and 10.

Regards,

On 3/9/2021 5:30 AM, Nuo Mi wrote:
> On Tue, Mar 9, 2021 at 3:17 AM Chad Fraleigh <chadf@triularity.org> wrote:
> 
>> On 2/14/2021 10:12 AM, Nuo Mi wrote:
>>> On Mon, Feb 15, 2021 at 2:08 AM Andreas Rheinhardt <
>>> andreas.rheinhardt@gmail.com> wrote:
>>>
>>>> Nuo Mi:
>>>>> ---
>>>>>    libavutil/opt.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>>>> index 590146b5fb..c554e9c063 100644
>>>>> --- a/libavutil/opt.c
>>>>> +++ b/libavutil/opt.c
>>>>> @@ -1735,7 +1735,7 @@ const AVClass *av_opt_child_class_iterate(const
>>>> AVClass *parent, void **iter)
>>>>>    #if FF_API_CHILD_CLASS_NEXT
>>>>>    FF_DISABLE_DEPRECATION_WARNINGS
>>>>>        if (parent->child_class_next) {
>>>>> -        *iter = parent->child_class_next(*iter);
>>>>> +        *iter = (void*)parent->child_class_next(*iter);
>>>>>            return *iter;
>>>>>        }
>>>>>    FF_ENABLE_DEPRECATION_WARNINGS
>>>>>
>>>> This doesn't look like a fix; you just silenced a warning.
>>>>
>>> Yes, but this the only way if we do not change the function signature.
>>
>> Since a major release is "coming up", now could be the time to fix the
>> signature to '(..., const void **iter)' and eliminate the non-const cast
>> kludge across the board for these cases.
> 
> Are we plan to remove all warnings before the release?
> Or more aggressively, is it feasible to treat all warnings as errors in the
> future? :)

I was thinking of just the 14 or so opt iterator functions which are 
assign to an AVClass.child_class_iterate (and related code in opt.c / 
opt.h / their iterate callers), so they no longer have to do a de-const 
(void *) cast to update *iter.

Though, it seems av_filter_iterate() and av_bsf_iterate() (maybe 
others?) would need to (/should?) be changed too, as they do the same 
non-const argument and are directly chained by child_class_iterate 
functions.