Re: [FFmpeg-devel] [PATCH 1/2] fftools: Use right function signature and pointers
On Wed, Aug 14, 2019 at 06:06:04PM +0200, Paul B Mahol wrote: > LGTM will apply thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already. signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 1/2] fftools: Use right function signature and pointers
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
> > ---
> > 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, {_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
Re: [FFmpeg-devel] [PATCH 1/2] fftools: Use right function signature and pointers
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
> ---
> 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, {_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) \
> { \
>
