diff mbox series

[FFmpeg-devel,1/3] dnn/queue: fix redefining typedefs

Message ID 20201230062637.29513-1-yejun.guo@intel.com
State New
Headers show
Series [FFmpeg-devel,1/3] dnn/queue: fix redefining typedefs
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

Guo, Yejun Dec. 30, 2020, 6:26 a.m. UTC
Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
 libavfilter/dnn/queue.c      | 8 ++++----
 libavfilter/dnn/safe_queue.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt Dec. 30, 2020, 10:03 a.m. UTC | #1
Guo, Yejun:
> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> ---
>  libavfilter/dnn/queue.c      | 8 ++++----
>  libavfilter/dnn/safe_queue.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c
> index 0a07c5473d..da0517968d 100644
> --- a/libavfilter/dnn/queue.c
> +++ b/libavfilter/dnn/queue.c
> @@ -25,17 +25,17 @@
>  
>  typedef struct _queue_entry queue_entry;

There is no point in using different type names and struct tags; and
both the type name as well as the struct tag do not abide by our naming
conventions: We use CamelCase for them. Remember
9beaf536fe5b52ed5af4d4dd5746277ee5ac9552? The deeper reason for this was
that the typedef name "thread_param" was masked by a variable of the
same name (of type thread_param *) and so sizeof(thread_param) referred
to the variable size (i.e. pointer size). Using sizeof with parentheses
didn't change this. If you had used our naming convention, this would
have never happened.

And both the non-internal type as well as the function names need the
correct prefix.

>  
> -typedef struct _queue {
> +struct _queue {
>      queue_entry *head;
>      queue_entry *tail;
>      size_t length;
> -}queue;
> +};

It is more logical to have this definition after the entry's definition.

>  
> -typedef struct _queue_entry {
> +struct _queue_entry {
>      void *value;
>      queue_entry *prev;
>      queue_entry *next;
> -} queue_entry;
> +};
>  
>  static inline queue_entry *create_entry(void *val)
>  {
> diff --git a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c
> index dba2e0fbbc..4298048454 100644
> --- a/libavfilter/dnn/safe_queue.c
> +++ b/libavfilter/dnn/safe_queue.c
> @@ -25,11 +25,11 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/thread.h"
>  
> -typedef struct _safe_queue {
> +struct _safe_queue {
>      queue *q;
>      pthread_mutex_t mutex;
>      pthread_cond_t cond;
> -}safe_queue;
> +};
>  
>  safe_queue *safe_queue_create(void)
>  {
>
Guo, Yejun Dec. 30, 2020, 12:57 p.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2020年12月30日 18:04
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining typedefs
> 
> Guo, Yejun:
> > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > ---
> >  libavfilter/dnn/queue.c      | 8 ++++----
> >  libavfilter/dnn/safe_queue.c | 4 ++--
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c index
> > 0a07c5473d..da0517968d 100644
> > --- a/libavfilter/dnn/queue.c
> > +++ b/libavfilter/dnn/queue.c
> > @@ -25,17 +25,17 @@
> >
> >  typedef struct _queue_entry queue_entry;
> 
> There is no point in using different type names and struct tags; and both the
> type name as well as the struct tag do not abide by our naming
> conventions: We use CamelCase for them. Remember
> 9beaf536fe5b52ed5af4d4dd5746277ee5ac9552? The deeper reason for this
> was that the typedef name "thread_param" was masked by a variable of the
> same name (of type thread_param *) and so sizeof(thread_param) referred to
> the variable size (i.e. pointer size). Using sizeof with parentheses didn't change
> this. If you had used our naming convention, this would have never happened.
thanks, got it. I'll also change thread_param in another patch after this regression is fixed.

> 
> And both the non-internal type as well as the function names need the correct
> prefix.
thanks, I'll add dnn_ as prefix for queue, safe_queue, and the relative functions.

> 
> >
> > -typedef struct _queue {
> > +struct _queue {
> >      queue_entry *head;
> >      queue_entry *tail;
> >      size_t length;
> > -}queue;
> > +};
> 
> It is more logical to have this definition after the entry's definition.
will change it, thanks.

> 
> >
> > -typedef struct _queue_entry {
> > +struct _queue_entry {
> >      void *value;
> >      queue_entry *prev;
> >      queue_entry *next;
> > -} queue_entry;
> > +};
> >
> >  static inline queue_entry *create_entry(void *val)  { diff --git
> > a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c index
> > dba2e0fbbc..4298048454 100644
> > --- a/libavfilter/dnn/safe_queue.c
> > +++ b/libavfilter/dnn/safe_queue.c
> > @@ -25,11 +25,11 @@
> >  #include "libavutil/avassert.h"
> >  #include "libavutil/thread.h"
> >
> > -typedef struct _safe_queue {
> > +struct _safe_queue {
> >      queue *q;
> >      pthread_mutex_t mutex;
> >      pthread_cond_t cond;
> > -}safe_queue;
> > +};
> >
> >  safe_queue *safe_queue_create(void)
> >  {
> >
> 
> _______________________________________________
> 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 Dec. 30, 2020, 1:03 p.m. UTC | #3
Guo, Yejun:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: 2020年12月30日 18:04
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining typedefs
>>
>> Guo, Yejun:
>>> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
>>> ---
>>>  libavfilter/dnn/queue.c      | 8 ++++----
>>>  libavfilter/dnn/safe_queue.c | 4 ++--
>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c index
>>> 0a07c5473d..da0517968d 100644
>>> --- a/libavfilter/dnn/queue.c
>>> +++ b/libavfilter/dnn/queue.c
>>> @@ -25,17 +25,17 @@
>>>
>>>  typedef struct _queue_entry queue_entry;
>>
>> There is no point in using different type names and struct tags; and both the
>> type name as well as the struct tag do not abide by our naming
>> conventions: We use CamelCase for them. Remember
>> 9beaf536fe5b52ed5af4d4dd5746277ee5ac9552? The deeper reason for this
>> was that the typedef name "thread_param" was masked by a variable of the
>> same name (of type thread_param *) and so sizeof(thread_param) referred to
>> the variable size (i.e. pointer size). Using sizeof with parentheses didn't change
>> this. If you had used our naming convention, this would have never happened.
> thanks, got it. I'll also change thread_param in another patch after this regression is fixed.
> 
>>
>> And both the non-internal type as well as the function names need the correct
>> prefix.
> thanks, I'll add dnn_ as prefix for queue, safe_queue, and the relative functions.
> 

The correct prefix for symbols not exported from the library and not
local to one translation unit is ff_ (or FF for types).

>>
>>>
>>> -typedef struct _queue {
>>> +struct _queue {
>>>      queue_entry *head;
>>>      queue_entry *tail;
>>>      size_t length;
>>> -}queue;
>>> +};
>>
>> It is more logical to have this definition after the entry's definition.
> will change it, thanks.
> 
>>
>>>
>>> -typedef struct _queue_entry {
>>> +struct _queue_entry {
>>>      void *value;
>>>      queue_entry *prev;
>>>      queue_entry *next;
>>> -} queue_entry;
>>> +};
>>>
>>>  static inline queue_entry *create_entry(void *val)  { diff --git
>>> a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c index
>>> dba2e0fbbc..4298048454 100644
>>> --- a/libavfilter/dnn/safe_queue.c
>>> +++ b/libavfilter/dnn/safe_queue.c
>>> @@ -25,11 +25,11 @@
>>>  #include "libavutil/avassert.h"
>>>  #include "libavutil/thread.h"
>>>
>>> -typedef struct _safe_queue {
>>> +struct _safe_queue {
>>>      queue *q;
>>>      pthread_mutex_t mutex;
>>>      pthread_cond_t cond;
>>> -}safe_queue;
>>> +};
>>>
>>>  safe_queue *safe_queue_create(void)
>>>  {
>>>
>>
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
>
Guo, Yejun Dec. 30, 2020, 1:04 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Guo,
> Yejun
> Sent: 2020年12月30日 20:58
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining typedefs
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Andreas Rheinhardt
> > Sent: 2020年12月30日 18:04
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining
> > typedefs
> >
> > Guo, Yejun:
> > > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > > ---
> > >  libavfilter/dnn/queue.c      | 8 ++++----
> > >  libavfilter/dnn/safe_queue.c | 4 ++--
> > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c index
> > > 0a07c5473d..da0517968d 100644
> > > --- a/libavfilter/dnn/queue.c
> > > +++ b/libavfilter/dnn/queue.c
> > > @@ -25,17 +25,17 @@
> > >
> > >  typedef struct _queue_entry queue_entry;
> >
> > There is no point in using different type names and struct tags; and
> > both the type name as well as the struct tag do not abide by our
> > naming
> > conventions: We use CamelCase for them. Remember
> > 9beaf536fe5b52ed5af4d4dd5746277ee5ac9552? The deeper reason for this
> > was that the typedef name "thread_param" was masked by a variable of
> > the same name (of type thread_param *) and so sizeof(thread_param)
> > referred to the variable size (i.e. pointer size). Using sizeof with
> > parentheses didn't change this. If you had used our naming convention, this
> would have never happened.
> thanks, got it. I'll also change thread_param in another patch after this
> regression is fixed.
> 
> >
> > And both the non-internal type as well as the function names need the
> > correct prefix.
> thanks, I'll add dnn_ as prefix for queue, safe_queue, and the relative functions.
DNNQueue, DNNSafeQueue, dnn_queue_create, etc

> 
> >
> > >
> > > -typedef struct _queue {
> > > +struct _queue {
> > >      queue_entry *head;
> > >      queue_entry *tail;
> > >      size_t length;
> > > -}queue;
> > > +};
> >
> > It is more logical to have this definition after the entry's definition.
> will change it, thanks.
> 
> >
> > >
> > > -typedef struct _queue_entry {
> > > +struct _queue_entry {
> > >      void *value;
> > >      queue_entry *prev;
> > >      queue_entry *next;
> > > -} queue_entry;
> > > +};
> > >
> > >  static inline queue_entry *create_entry(void *val)  { diff --git
> > > a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c index
> > > dba2e0fbbc..4298048454 100644
> > > --- a/libavfilter/dnn/safe_queue.c
> > > +++ b/libavfilter/dnn/safe_queue.c
> > > @@ -25,11 +25,11 @@
> > >  #include "libavutil/avassert.h"
> > >  #include "libavutil/thread.h"
> > >
> > > -typedef struct _safe_queue {
> > > +struct _safe_queue {
> > >      queue *q;
> > >      pthread_mutex_t mutex;
> > >      pthread_cond_t cond;
> > > -}safe_queue;
> > > +};
> > >
> > >  safe_queue *safe_queue_create(void)  {
> > >
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
Guo, Yejun Dec. 30, 2020, 1:06 p.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: 2020年12月30日 21:03
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining typedefs
> 
> Guo, Yejun:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: 2020年12月30日 18:04
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] dnn/queue: fix redefining
> >> typedefs
> >>
> >> Guo, Yejun:
> >>> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> >>> ---
> >>>  libavfilter/dnn/queue.c      | 8 ++++----
> >>>  libavfilter/dnn/safe_queue.c | 4 ++--
> >>>  2 files changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c index
> >>> 0a07c5473d..da0517968d 100644
> >>> --- a/libavfilter/dnn/queue.c
> >>> +++ b/libavfilter/dnn/queue.c
> >>> @@ -25,17 +25,17 @@
> >>>
> >>>  typedef struct _queue_entry queue_entry;
> >>
> >> There is no point in using different type names and struct tags; and
> >> both the type name as well as the struct tag do not abide by our
> >> naming
> >> conventions: We use CamelCase for them. Remember
> >> 9beaf536fe5b52ed5af4d4dd5746277ee5ac9552? The deeper reason for
> this
> >> was that the typedef name "thread_param" was masked by a variable of
> >> the same name (of type thread_param *) and so sizeof(thread_param)
> >> referred to the variable size (i.e. pointer size). Using sizeof with
> >> parentheses didn't change this. If you had used our naming convention, this
> would have never happened.
> > thanks, got it. I'll also change thread_param in another patch after this
> regression is fixed.
> >
> >>
> >> And both the non-internal type as well as the function names need the
> >> correct prefix.
> > thanks, I'll add dnn_ as prefix for queue, safe_queue, and the relative
> functions.
> >
> 
> The correct prefix for symbols not exported from the library and not local to
> one translation unit is ff_ (or FF for types).
got it, it will be FFQueue, FFSafeQueue, ff_safe_queue_size, ff_queue_create, etc. thanks.

> 
> >>
> >>>
> >>> -typedef struct _queue {
> >>> +struct _queue {
> >>>      queue_entry *head;
> >>>      queue_entry *tail;
> >>>      size_t length;
> >>> -}queue;
> >>> +};
> >>
> >> It is more logical to have this definition after the entry's definition.
> > will change it, thanks.
> >
> >>
> >>>
> >>> -typedef struct _queue_entry {
> >>> +struct _queue_entry {
> >>>      void *value;
> >>>      queue_entry *prev;
> >>>      queue_entry *next;
> >>> -} queue_entry;
> >>> +};
> >>>
> >>>  static inline queue_entry *create_entry(void *val)  { diff --git
> >>> a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c index
> >>> dba2e0fbbc..4298048454 100644
> >>> --- a/libavfilter/dnn/safe_queue.c
> >>> +++ b/libavfilter/dnn/safe_queue.c
> >>> @@ -25,11 +25,11 @@
> >>>  #include "libavutil/avassert.h"
> >>>  #include "libavutil/thread.h"
> >>>
> >>> -typedef struct _safe_queue {
> >>> +struct _safe_queue {
> >>>      queue *q;
> >>>      pthread_mutex_t mutex;
> >>>      pthread_cond_t cond;
> >>> -}safe_queue;
> >>> +};
> >>>
> >>>  safe_queue *safe_queue_create(void)  {
> >>>
> >>
> >> _______________________________________________
> >> 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".
> > _______________________________________________
> > 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".
> >
> 
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavfilter/dnn/queue.c b/libavfilter/dnn/queue.c
index 0a07c5473d..da0517968d 100644
--- a/libavfilter/dnn/queue.c
+++ b/libavfilter/dnn/queue.c
@@ -25,17 +25,17 @@ 
 
 typedef struct _queue_entry queue_entry;
 
-typedef struct _queue {
+struct _queue {
     queue_entry *head;
     queue_entry *tail;
     size_t length;
-}queue;
+};
 
-typedef struct _queue_entry {
+struct _queue_entry {
     void *value;
     queue_entry *prev;
     queue_entry *next;
-} queue_entry;
+};
 
 static inline queue_entry *create_entry(void *val)
 {
diff --git a/libavfilter/dnn/safe_queue.c b/libavfilter/dnn/safe_queue.c
index dba2e0fbbc..4298048454 100644
--- a/libavfilter/dnn/safe_queue.c
+++ b/libavfilter/dnn/safe_queue.c
@@ -25,11 +25,11 @@ 
 #include "libavutil/avassert.h"
 #include "libavutil/thread.h"
 
-typedef struct _safe_queue {
+struct _safe_queue {
     queue *q;
     pthread_mutex_t mutex;
     pthread_cond_t cond;
-}safe_queue;
+};
 
 safe_queue *safe_queue_create(void)
 {