Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sun, Oct 25, 2015 at 1:03 PM, Michael Niedermayer wrote: > On Sun, Oct 25, 2015 at 12:46:31PM -0400, Ganesh Ajjanagadde wrote: >> On Sun, Oct 25, 2015 at 12:35 PM, Michael Niedermayer >> wrote: >> > On Sat, Oct 24, 2015 at 09:03:03PM -0400, Ganesh Ajjanagadde wrote: >> >> On Sat, Oct 24, 2015 at 7:17 PM, Henrik Gramner >> >> wrote: >> >> > On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde >> >> > wrote: >> >> >> -static int cmp(void *key, const void *node) >> >> >> +static int cmp(const void *key, const void *node) >> >> >> { >> >> >> return (*(int64_t *) key) - ((const CacheEntry *) >> >> >> node)->logical_pos; >> >> >> } >> >> > >> >> >> -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) >> >> >> +int ff_nut_sp_pos_cmp(const void *a, const void *b) >> >> >> { >> >> >> -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); >> >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> >> >> +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); >> >> >> } >> >> > >> >> >> -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) >> >> >> +int ff_nut_sp_pts_cmp(const void *a, const void *b) >> >> >> { >> >> >> -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); >> >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> >> >> +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); >> >> >> } >> >> > >> >> > Casts discards const qualifiers. >> >> >> >> Good catch, changed. There is some more such constness being discarded >> >> in comparators, and some needlessly complex comparator logic in some >> >> places. Submitted a patch. >> >> >> >> > >> >> > Furthermore, why are you changing the two last functions to copy the >> >> > entire struct to a temporary local copy? >> >> >> >> Sorry, forgot they were structs. >> >> Fixed all, pushed. Thanks. >> > >> > i think this results in some new warnings for the tree test build >> > >> > libavutil/tree.c: In function ‘main’: >> > libavutil/tree.c:238:9: warning: passing argument 3 of ‘av_tree_insert’ >> > from incompatible pointer type [enabled by default] >> > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void >> > *)’ but argument is of type ‘int (*)(void *, const void *)’ >> > libavutil/tree.c:244:13: warning: passing argument 3 of ‘av_tree_insert’ >> > from incompatible pointer type [enabled by default] >> > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void >> > *)’ but argument is of type ‘int (*)(void *, const void *)’ >> > libavutil/tree.c:245:13: warning: passing argument 3 of ‘av_tree_find’ >> > from incompatible pointer type [enabled by default] >> >> Easy fix, and pushed. Sorry. > >> BTW, how did you enable the test build? > > make libavutil/tree-test > or > make fate > > builds it Thanks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User > questions about the command line tools should be sent to the ffmpeg-user ML. > And questions about how to use libav* should be sent to the libav-user ML. > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sun, Oct 25, 2015 at 12:46:31PM -0400, Ganesh Ajjanagadde wrote: > On Sun, Oct 25, 2015 at 12:35 PM, Michael Niedermayer > wrote: > > On Sat, Oct 24, 2015 at 09:03:03PM -0400, Ganesh Ajjanagadde wrote: > >> On Sat, Oct 24, 2015 at 7:17 PM, Henrik Gramner wrote: > >> > On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde > >> > wrote: > >> >> -static int cmp(void *key, const void *node) > >> >> +static int cmp(const void *key, const void *node) > >> >> { > >> >> return (*(int64_t *) key) - ((const CacheEntry *) > >> >> node)->logical_pos; > >> >> } > >> > > >> >> -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) > >> >> +int ff_nut_sp_pos_cmp(const void *a, const void *b) > >> >> { > >> >> -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); > >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > >> >> +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); > >> >> } > >> > > >> >> -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) > >> >> +int ff_nut_sp_pts_cmp(const void *a, const void *b) > >> >> { > >> >> -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); > >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > >> >> +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); > >> >> } > >> > > >> > Casts discards const qualifiers. > >> > >> Good catch, changed. There is some more such constness being discarded > >> in comparators, and some needlessly complex comparator logic in some > >> places. Submitted a patch. > >> > >> > > >> > Furthermore, why are you changing the two last functions to copy the > >> > entire struct to a temporary local copy? > >> > >> Sorry, forgot they were structs. > >> Fixed all, pushed. Thanks. > > > > i think this results in some new warnings for the tree test build > > > > libavutil/tree.c: In function ‘main’: > > libavutil/tree.c:238:9: warning: passing argument 3 of ‘av_tree_insert’ > > from incompatible pointer type [enabled by default] > > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ > > but argument is of type ‘int (*)(void *, const void *)’ > > libavutil/tree.c:244:13: warning: passing argument 3 of ‘av_tree_insert’ > > from incompatible pointer type [enabled by default] > > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ > > but argument is of type ‘int (*)(void *, const void *)’ > > libavutil/tree.c:245:13: warning: passing argument 3 of ‘av_tree_find’ from > > incompatible pointer type [enabled by default] > > Easy fix, and pushed. Sorry. > BTW, how did you enable the test build? make libavutil/tree-test or make fate builds it [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sun, Oct 25, 2015 at 12:35 PM, Michael Niedermayer wrote: > On Sat, Oct 24, 2015 at 09:03:03PM -0400, Ganesh Ajjanagadde wrote: >> On Sat, Oct 24, 2015 at 7:17 PM, Henrik Gramner wrote: >> > On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde >> > wrote: >> >> -static int cmp(void *key, const void *node) >> >> +static int cmp(const void *key, const void *node) >> >> { >> >> return (*(int64_t *) key) - ((const CacheEntry *) node)->logical_pos; >> >> } >> > >> >> -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) >> >> +int ff_nut_sp_pos_cmp(const void *a, const void *b) >> >> { >> >> -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> >> +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); >> >> } >> > >> >> -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) >> >> +int ff_nut_sp_pts_cmp(const void *a, const void *b) >> >> { >> >> -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); >> >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> >> +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); >> >> } >> > >> > Casts discards const qualifiers. >> >> Good catch, changed. There is some more such constness being discarded >> in comparators, and some needlessly complex comparator logic in some >> places. Submitted a patch. >> >> > >> > Furthermore, why are you changing the two last functions to copy the >> > entire struct to a temporary local copy? >> >> Sorry, forgot they were structs. >> Fixed all, pushed. Thanks. > > i think this results in some new warnings for the tree test build > > libavutil/tree.c: In function ‘main’: > libavutil/tree.c:238:9: warning: passing argument 3 of ‘av_tree_insert’ from > incompatible pointer type [enabled by default] > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ > but argument is of type ‘int (*)(void *, const void *)’ > libavutil/tree.c:244:13: warning: passing argument 3 of ‘av_tree_insert’ from > incompatible pointer type [enabled by default] > libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ > but argument is of type ‘int (*)(void *, const void *)’ > libavutil/tree.c:245:13: warning: passing argument 3 of ‘av_tree_find’ from > incompatible pointer type [enabled by default] Easy fix, and pushed. Sorry. BTW, how did you enable the test build? > > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Freedom in capitalist society always remains about the same as it was in > ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sat, Oct 24, 2015 at 09:03:03PM -0400, Ganesh Ajjanagadde wrote: > On Sat, Oct 24, 2015 at 7:17 PM, Henrik Gramner wrote: > > On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde > > wrote: > >> -static int cmp(void *key, const void *node) > >> +static int cmp(const void *key, const void *node) > >> { > >> return (*(int64_t *) key) - ((const CacheEntry *) node)->logical_pos; > >> } > > > >> -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) > >> +int ff_nut_sp_pos_cmp(const void *a, const void *b) > >> { > >> -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); > >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > >> +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); > >> } > > > >> -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) > >> +int ff_nut_sp_pts_cmp(const void *a, const void *b) > >> { > >> -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); > >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > >> +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); > >> } > > > > Casts discards const qualifiers. > > Good catch, changed. There is some more such constness being discarded > in comparators, and some needlessly complex comparator logic in some > places. Submitted a patch. > > > > > Furthermore, why are you changing the two last functions to copy the > > entire struct to a temporary local copy? > > Sorry, forgot they were structs. > Fixed all, pushed. Thanks. i think this results in some new warnings for the tree test build libavutil/tree.c: In function ‘main’: libavutil/tree.c:238:9: warning: passing argument 3 of ‘av_tree_insert’ from incompatible pointer type [enabled by default] libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ but argument is of type ‘int (*)(void *, const void *)’ libavutil/tree.c:244:13: warning: passing argument 3 of ‘av_tree_insert’ from incompatible pointer type [enabled by default] libavutil/tree.c:59:7: note: expected ‘int (*)(const void *, const void *)’ but argument is of type ‘int (*)(void *, const void *)’ libavutil/tree.c:245:13: warning: passing argument 3 of ‘av_tree_find’ from incompatible pointer type [enabled by default] [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sat, Oct 24, 2015 at 7:17 PM, Henrik Gramner wrote: > On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde > wrote: >> -static int cmp(void *key, const void *node) >> +static int cmp(const void *key, const void *node) >> { >> return (*(int64_t *) key) - ((const CacheEntry *) node)->logical_pos; >> } > >> -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) >> +int ff_nut_sp_pos_cmp(const void *a, const void *b) >> { >> -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); >> } > >> -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) >> +int ff_nut_sp_pts_cmp(const void *a, const void *b) >> { >> -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); >> +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; >> +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); >> } > > Casts discards const qualifiers. Good catch, changed. There is some more such constness being discarded in comparators, and some needlessly complex comparator logic in some places. Submitted a patch. > > Furthermore, why are you changing the two last functions to copy the > entire struct to a temporary local copy? Sorry, forgot they were structs. Fixed all, pushed. Thanks. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
On Sun, Oct 25, 2015 at 12:02 AM, Ganesh Ajjanagadde wrote: > -static int cmp(void *key, const void *node) > +static int cmp(const void *key, const void *node) > { > return (*(int64_t *) key) - ((const CacheEntry *) node)->logical_pos; > } > -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) > +int ff_nut_sp_pos_cmp(const void *a, const void *b) > { > -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); > +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); > } > -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) > +int ff_nut_sp_pts_cmp(const void *a, const void *b) > { > -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); > +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; > +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); > } Casts discards const qualifiers. Furthermore, why are you changing the two last functions to copy the entire struct to a temporary local copy? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] avutil/tree: add additional const qualifier to the comparator
libc's qsort comparator has a const qualifier on both arguments. This adds a missing const qualifier to exactly match the comparator API. Existing usages of av_tree_find, av_tree_insert are appropriately modified: type signature changes of the comparators, and removal of unnecessary void * casts of function pointers. Signed-off-by: Ganesh Ajjanagadde --- libavfilter/vf_drawtext.c | 2 +- libavformat/cache.c | 2 +- libavformat/nut.c | 12 +++- libavformat/nut.h | 4 ++-- libavformat/nutdec.c | 6 +++--- libavformat/nutenc.c | 2 +- libavutil/tree.c | 4 ++-- libavutil/tree.h | 4 ++-- 8 files changed, 19 insertions(+), 17 deletions(-) diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 5c4676e..fc77be4 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -288,7 +288,7 @@ typedef struct Glyph { int bitmap_top; } Glyph; -static int glyph_cmp(void *key, const void *b) +static int glyph_cmp(const void *key, const void *b) { const Glyph *a = key, *bb = b; int64_t diff = (int64_t)a->code - (int64_t)bb->code; diff --git a/libavformat/cache.c b/libavformat/cache.c index d3d12bb..7f6b6e4 100644 --- a/libavformat/cache.c +++ b/libavformat/cache.c @@ -65,7 +65,7 @@ typedef struct Context { int read_ahead_limit; } Context; -static int cmp(void *key, const void *node) +static int cmp(const void *key, const void *node) { return (*(int64_t *) key) - ((const CacheEntry *) node)->logical_pos; } diff --git a/libavformat/nut.c b/libavformat/nut.c index c6fdb0b..b68ed6e 100644 --- a/libavformat/nut.c +++ b/libavformat/nut.c @@ -237,14 +237,16 @@ int64_t ff_lsb2full(StreamContext *stream, int64_t lsb) return ((lsb - delta) & mask) + delta; } -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b) +int ff_nut_sp_pos_cmp(const void *a, const void *b) { -return ((a->pos - b->pos) >> 32) - ((b->pos - a->pos) >> 32); +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; +return ((va.pos - vb.pos) >> 32) - ((vb.pos - va.pos) >> 32); } -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b) +int ff_nut_sp_pts_cmp(const void *a, const void *b) { -return ((a->ts - b->ts) >> 32) - ((b->ts - a->ts) >> 32); +Syncpoint va = *(Syncpoint *)a, vb = *(Syncpoint *)b; +return ((va.ts - vb.ts) >> 32) - ((vb.ts - va.ts) >> 32); } int ff_nut_add_sp(NUTContext *nut, int64_t pos, int64_t back_ptr, int64_t ts) @@ -263,7 +265,7 @@ int ff_nut_add_sp(NUTContext *nut, int64_t pos, int64_t back_ptr, int64_t ts) sp->pos = pos; sp->back_ptr = back_ptr; sp->ts = ts; -av_tree_insert(&nut->syncpoints, sp, (void *) ff_nut_sp_pos_cmp, &node); +av_tree_insert(&nut->syncpoints, sp, ff_nut_sp_pos_cmp, &node); if (node) { av_free(sp); av_free(node); diff --git a/libavformat/nut.h b/libavformat/nut.h index 45aa55c..a4409ee 100644 --- a/libavformat/nut.h +++ b/libavformat/nut.h @@ -132,8 +132,8 @@ typedef struct Dispositions { void ff_nut_reset_ts(NUTContext *nut, AVRational time_base, int64_t val); int64_t ff_lsb2full(StreamContext *stream, int64_t lsb); -int ff_nut_sp_pos_cmp(const Syncpoint *a, const Syncpoint *b); -int ff_nut_sp_pts_cmp(const Syncpoint *a, const Syncpoint *b); +int ff_nut_sp_pos_cmp(const void *a, const void *b); +int ff_nut_sp_pts_cmp(const void *a, const void *b); int ff_nut_add_sp(NUTContext *nut, int64_t pos, int64_t back_ptr, int64_t ts); void ff_nut_free_sp(NUTContext *nut); diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c index 63b0cd2..deceb03 100644 --- a/libavformat/nutdec.c +++ b/libavformat/nutdec.c @@ -1271,7 +1271,7 @@ static int read_seek(AVFormatContext *s, int stream_index, pos2 = st->index_entries[index].pos; ts = st->index_entries[index].timestamp; } else { -av_tree_find(nut->syncpoints, &dummy, (void *) ff_nut_sp_pts_cmp, +av_tree_find(nut->syncpoints, &dummy, ff_nut_sp_pts_cmp, (void **) next_node); av_log(s, AV_LOG_DEBUG, "%"PRIu64"-%"PRIu64" %"PRId64"-%"PRId64"\n", next_node[0]->pos, next_node[1]->pos, next_node[0]->ts, @@ -1286,7 +1286,7 @@ static int read_seek(AVFormatContext *s, int stream_index, if (!(flags & AVSEEK_FLAG_BACKWARD)) { dummy.pos= pos + 16; next_node[1] = &nopts_sp; -av_tree_find(nut->syncpoints, &dummy, (void *) ff_nut_sp_pos_cmp, +av_tree_find(nut->syncpoints, &dummy, ff_nut_sp_pos_cmp, (void **) next_node); pos2 = ff_gen_search(s, -2, dummy.pos, next_node[0]->pos, next_node[1]->pos, next_node[1]->pos, @@ -1297,7 +1297,7 @@ static int read_seek(AVFormatContext *s, int stream_index, // FIXME dir but I think it does not matter } dummy.pos = pos; -sp = av_tree_find(nut->syncpoints