[FFmpeg-devel] lavu/threadmessage: Properly declare a function pointer

Hi!

Attached patch, inspired by a patch by Andreas, fixes the following
warning when -Wpedantic is used:
CC      libavutil/threadmessage.o
libavutil/threadmessage.c: In function ‘av_thread_message_flush’:
libavutil/threadmessage.c:222:23: warning: ISO C forbids
initialization between function pointer and ‘void *’ [-Wpedantic]
  222 |     void *free_func = mq->free_func;
      |                       ^~


Please comment, Carl Eugen

Carl Eugen Hoyos (12020-05-01):
> Hi!
> 
> Attached patch, inspired by a patch by Andreas, fixes the following
> warning when -Wpedantic is used:
> CC      libavutil/threadmessage.o
> libavutil/threadmessage.c: In function ‘av_thread_message_flush’:
> libavutil/threadmessage.c:222:23: warning: ISO C forbids
> initialization between function pointer and ‘void *’ [-Wpedantic]
>   222 |     void *free_func = mq->free_func;
>       |                       ^~
> 
> 
> Please comment, Carl Eugen

> From 3f0b6c654b7473452638c1cc06dfe45eebb59079 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Fri, 1 May 2020 23:42:01 +0200
> Subject: [PATCH] lavu/threadmessage: Properly declare a function pointer.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Fixes a pedantic warning:
> libavutil/threadmessage.c:222:23: warning: ISO C forbids initialization between function pointer and ‘void *’
> ---
>  libavutil/threadmessage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
> index 764b7fb813..797ba6c44c 100644
> --- a/libavutil/threadmessage.c
> +++ b/libavutil/threadmessage.c
> @@ -219,7 +219,7 @@ void av_thread_message_flush(AVThreadMessageQueue *mq)
>  {
>  #if HAVE_THREADS
>      int used, off;
> -    void *free_func = mq->free_func;
> +    void(*free_func)(void *) = mq->free_func;
>  
>      pthread_mutex_lock(&mq->lock);
>      used = av_fifo_size(mq->fifo);

This is not ok. Now free_func is properly a function pointer, but it is
later still used as a void *, it is as much invalid as it was before.

I thin it could work:

    void *free_funcp = &mq->free_func; // pointer to data

and in free_func_wrap():

void (*free_func)(void *) = *((void (*)(void *)) *)arg;

But that needs testing.

Do we support an arch where function pointers are different?

Regards,

Am Sa., 2. Mai 2020 um 11:06 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Carl Eugen Hoyos (12020-05-01):
> > Hi!
> >
> > Attached patch, inspired by a patch by Andreas, fixes the following
> > warning when -Wpedantic is used:
> > CC      libavutil/threadmessage.o
> > libavutil/threadmessage.c: In function ‘av_thread_message_flush’:
> > libavutil/threadmessage.c:222:23: warning: ISO C forbids
> > initialization between function pointer and ‘void *’ [-Wpedantic]
> >   222 |     void *free_func = mq->free_func;
> >       |                       ^~
> >
> >
> > Please comment, Carl Eugen
>
> > From 3f0b6c654b7473452638c1cc06dfe45eebb59079 Mon Sep 17 00:00:00 2001
> > From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > Date: Fri, 1 May 2020 23:42:01 +0200
> > Subject: [PATCH] lavu/threadmessage: Properly declare a function pointer.
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> >
> > Fixes a pedantic warning:
> > libavutil/threadmessage.c:222:23: warning: ISO C forbids initialization between function pointer and ‘void *’
> > ---
> >  libavutil/threadmessage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
> > index 764b7fb813..797ba6c44c 100644
> > --- a/libavutil/threadmessage.c
> > +++ b/libavutil/threadmessage.c
> > @@ -219,7 +219,7 @@ void av_thread_message_flush(AVThreadMessageQueue *mq)
> >  {
> >  #if HAVE_THREADS
> >      int used, off;
> > -    void *free_func = mq->free_func;
> > +    void(*free_func)(void *) = mq->free_func;
> >
> >      pthread_mutex_lock(&mq->lock);
> >      used = av_fifo_size(mq->fifo);
>
> This is not ok. Now free_func is properly a function pointer, but it is
> later still used as a void *, it is as much invalid as it was before.

Could you tell me where free_func() is used as a void *?
I don't see it.

Carl Eugen

Carl Eugen Hoyos (12020-05-02):
> Could you tell me where free_func() is used as a void *?
> I don't see it.

You are right, I misread. The problem is even simpler: why is there an
intermediate variable in the first place? Just test mq->free_func
directly.

Regards,

Am Sa., 2. Mai 2020 um 11:56 Uhr schrieb Nicolas George <george@nsup.org>:
>
> Carl Eugen Hoyos (12020-05-02):
> > Could you tell me where free_func() is used as a void *?
> > I don't see it.
>
> You are right, I misread. The problem is even simpler: why is there an
> intermediate variable in the first place?

My guess is to avoid a race condition.

Carl Eugen

Carl Eugen Hoyos (12020-05-02):
> My guess is to avoid a race condition.

Avoid a race condition is: take local variable, unlock, use local
variable. Here it is take local variable, lock, use local variable.

Regards,