LGTM On Mon, Aug 12, 2019 at 11:59 PM Andreas Rheinhardt < andreas.rheinha...@gmail.com> wrote:
> Andreas Rheinhardt: > > The option tables of the various fftools (in particular ffprobe) are > > arrays of OptionDef; said type contains a union of a pointer to void and > > a function pointer of type int (*)(void *, const char *, const char *) > > as well as a size_t. Some entries (namely the common entry for writing a > > report as well as several more of ffprobe's entries) used the pointer to > > void to store a pointer to functions of type int (*)(const char *) or > > type int (*)(const char *, const char *); nevertheless, when the > functions > > are actually called in write_option (in cmdutils.c), it is done via a > > pointer of the first type. > > > > There are two things wrong here: > > 1. Pointer to void can be converted to any pointer to incomplete or > > object type and back; but they are nevertheless not completely generic > > pointers: There is no provision in the C standard that guarantees their > > convertibility with function pointers. C90 lacks a generic function > > pointer, C99 made every function pointer a generic function pointer and > > still disallows the convertibility with void *. > > 2. The signature of the called function differs from the signature > > of the pointed-to type. This is undefined behaviour in C99 (given that > > C90 lacks a way to convert function pointers at all, it doesn't say > > anything about such a situation). It only works because none of the > > functions this patch is about make any use of their parameters at all. > > > > Therefore this commit changes the type of the relevant functions > > to match the type used for the call and uses the union's function > > pointer to store it. This is legal even in C90. > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> > > --- > > There are other places in the codebase that use a pointer to void to > > store a function pointer. Should they be changed or not? > > > > fftools/cmdutils.c | 2 +- > > fftools/cmdutils.h | 4 ++-- > > fftools/ffprobe.c | 26 +++++++++++++------------- > > 3 files changed, 16 insertions(+), 16 deletions(-) > > > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > > index 9cfbc45c2b..fdcd376b76 100644 > > --- a/fftools/cmdutils.c > > +++ b/fftools/cmdutils.c > > @@ -1046,7 +1046,7 @@ static int init_report(const char *env) > > return 0; > > } > > > > -int opt_report(const char *opt) > > +int opt_report(void *optctx, const char *opt, const char *arg) > > { > > return init_report(NULL); > > } > > diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h > > index 6e2e0a2acb..1917510589 100644 > > --- a/fftools/cmdutils.h > > +++ b/fftools/cmdutils.h > > @@ -99,7 +99,7 @@ int opt_default(void *optctx, const char *opt, const > char *arg); > > */ > > int opt_loglevel(void *optctx, const char *opt, const char *arg); > > > > -int opt_report(const char *opt); > > +int opt_report(void *optctx, const char *opt, const char *arg); > > > > int opt_max_alloc(void *optctx, const char *opt, const char *arg); > > > > @@ -236,7 +236,7 @@ void show_help_options(const OptionDef *options, > const char *msg, int req_flags, > > { "colors", OPT_EXIT, { .func_arg = show_colors > }, "show available color names" }, \ > > { "loglevel", HAS_ARG, { .func_arg = opt_loglevel > }, "set logging level", "loglevel" }, \ > > { "v", HAS_ARG, { .func_arg = opt_loglevel > }, "set logging level", "loglevel" }, \ > > - { "report", 0, { (void*)opt_report }, > "generate a report" }, \ > > + { "report", 0, { .func_arg = opt_report }, > "generate a report" }, \ > > { "max_alloc", HAS_ARG, { .func_arg = opt_max_alloc > }, "set maximum size of a single allocated block", "bytes" }, \ > > { "cpuflags", HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags > }, "force specific cpu flags", "flags" }, \ > > { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner}, "do not > show program banner", "hide_banner" }, \ > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > > index 5aaddb0308..2380417229 100644 > > --- a/fftools/ffprobe.c > > +++ b/fftools/ffprobe.c > > @@ -3471,7 +3471,7 @@ static int opt_sections(void *optctx, const char > *opt, const char *arg) > > return 0; > > } > > > > -static int opt_show_versions(const char *opt, const char *arg) > > +static int opt_show_versions(void *optctx, const char *opt, const char > *arg) > > { > > mark_section_show_entries(SECTION_ID_PROGRAM_VERSION, 1, NULL); > > mark_section_show_entries(SECTION_ID_LIBRARY_VERSION, 1, NULL); > > @@ -3479,7 +3479,7 @@ static int opt_show_versions(const char *opt, > const char *arg) > > } > > > > #define DEFINE_OPT_SHOW_SECTION(section, target_section_id) > \ > > - static int opt_show_##section(const char *opt, const char *arg) > \ > > + static int opt_show_##section(void *optctx, const char *opt, const > char *arg) \ > > { > \ > > mark_section_show_entries(SECTION_ID_##target_section_id, 1, > NULL); \ > > return 0; > \ > > @@ -3514,9 +3514,9 @@ static const OptionDef real_options[] = { > > { "sections", OPT_EXIT, {.func_arg = opt_sections}, "print sections > structure and section information, and exit" }, > > { "show_data", OPT_BOOL, {(void*)&do_show_data}, "show packets > data" }, > > { "show_data_hash", OPT_STRING | HAS_ARG, {(void*)&show_data_hash}, > "show packets data hash" }, > > - { "show_error", 0, {(void*)&opt_show_error}, "show probing > error" }, > > - { "show_format", 0, {(void*)&opt_show_format}, "show > format/container info" }, > > - { "show_frames", 0, {(void*)&opt_show_frames}, "show frames info" > }, > > + { "show_error", 0, { .func_arg = &opt_show_error }, "show > probing error" }, > > + { "show_format", 0, { .func_arg = &opt_show_format }, "show > format/container info" }, > > + { "show_frames", 0, { .func_arg = &opt_show_frames }, "show frames > info" }, > > { "show_format_entry", HAS_ARG, {.func_arg = opt_show_format_entry}, > > "show a particular entry from the format/container info", "entry" > }, > > { "show_entries", HAS_ARG, {.func_arg = opt_show_entries}, > > @@ -3524,16 +3524,16 @@ static const OptionDef real_options[] = { > > #if HAVE_THREADS > > { "show_log", OPT_INT|HAS_ARG, {(void*)&do_show_log}, "show log" }, > > #endif > > - { "show_packets", 0, {(void*)&opt_show_packets}, "show packets > info" }, > > - { "show_programs", 0, {(void*)&opt_show_programs}, "show programs > info" }, > > - { "show_streams", 0, {(void*)&opt_show_streams}, "show streams > info" }, > > - { "show_chapters", 0, {(void*)&opt_show_chapters}, "show chapters > info" }, > > + { "show_packets", 0, { .func_arg = &opt_show_packets }, "show > packets info" }, > > + { "show_programs", 0, { .func_arg = &opt_show_programs }, "show > programs info" }, > > + { "show_streams", 0, { .func_arg = &opt_show_streams }, "show > streams info" }, > > + { "show_chapters", 0, { .func_arg = &opt_show_chapters }, "show > chapters info" }, > > { "count_frames", OPT_BOOL, {(void*)&do_count_frames}, "count the > number of frames per stream" }, > > { "count_packets", OPT_BOOL, {(void*)&do_count_packets}, "count the > number of packets per stream" }, > > - { "show_program_version", 0, {(void*)&opt_show_program_version}, > "show ffprobe version" }, > > - { "show_library_versions", 0, {(void*)&opt_show_library_versions}, > "show library versions" }, > > - { "show_versions", 0, {(void*)&opt_show_versions}, "show > program and library versions" }, > > - { "show_pixel_formats", 0, {(void*)&opt_show_pixel_formats}, "show > pixel format descriptions" }, > > + { "show_program_version", 0, { .func_arg = > &opt_show_program_version }, "show ffprobe version" }, > > + { "show_library_versions", 0, { .func_arg = > &opt_show_library_versions }, "show library versions" }, > > + { "show_versions", 0, { .func_arg = &opt_show_versions }, > "show program and library versions" }, > > + { "show_pixel_formats", 0, { .func_arg = &opt_show_pixel_formats }, > "show pixel format descriptions" }, > > { "show_private_data", OPT_BOOL, {(void*)&show_private_data}, "show > private data" }, > > { "private", OPT_BOOL, {(void*)&show_private_data}, "same > as show_private_data" }, > > { "bitexact", OPT_BOOL, {&do_bitexact}, "force bitexact output" }, > > > Ping. > > - 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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".