diff mbox series

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

Message ID 20210214165403.13554-1-nuomi2021@gmail.com
State New
Headers show
Series [FFmpeg-devel] avutils/opt: fix discarded-qualifiers compiler warning
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Nuo Mi Feb. 14, 2021, 4:54 p.m. UTC
---
 libavutil/opt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt Feb. 14, 2021, 6:08 p.m. UTC | #1
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
Nuo Mi Feb. 14, 2021, 6:12 p.m. UTC | #2
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".
Andreas Rheinhardt Feb. 14, 2021, 6:16 p.m. UTC | #3
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
Chad Fraleigh Feb. 14, 2021, 8:16 p.m. UTC | #4
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.
Chad Fraleigh March 8, 2021, 7:17 p.m. UTC | #5
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
Nuo Mi March 9, 2021, 1:30 p.m. UTC | #6
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".
Nicolas George March 9, 2021, 2 p.m. UTC | #7
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,
Chad Fraleigh March 9, 2021, 9:17 p.m. UTC | #8
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.
diff mbox series

Patch

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