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