[MPlayer-dev-eng] [PATCH] m_property.c - please give feedback

[Reimar Döffinger]

Hello,
On Sat, Nov 24, 2007 at 01:25:09AM -0800, Chris Welton wrote:

> Index: m_property.c
> ===================================================================
> --- m_property.c	(revision 25155)
> +++ m_property.c	(working copy)
> @@ -20,7 +20,9 @@
>  static int do_action(m_option_t* prop_list, const char* name,
>                       int action, void* arg, void *ctx) {
>      const char* sep;
> -    m_option_t* prop;
> +    const m_option_t* prop;
> +    const m_option_t** arg_handle;
> +    m_property_ctrl_f ctrl_f;
>      m_property_action_t ka;
>      int r;
>      if((sep = strchr(name,'/')) && sep[1]) {
> @@ -37,10 +39,12 @@
>      } else
>          prop = m_option_list_find(prop_list, name);
>      if(!prop) return M_PROPERTY_UNKNOWN;
> -    r = ((m_property_ctrl_f)prop->p)(prop,action,arg,ctx);
> +    ctrl_f = prop->p;
> +    r = ctrl_f((void *)prop,action,arg,ctx);

I do not see any advantages in using an extra ctrl_f variable.
In addition that cast to void * is not good, since it casts a const
away. There are only two ways to fix warnings: fix the code _properly_
(and casting away const is never proper, changing m_property_ctrl_f
instead probably would work fine here) or disable the warning.
Otherwise changing m_option_t* prop; to const is correct.

>      if(action == M_PROPERTY_GET_TYPE && r < 0) {
>         if(!arg) return M_PROPERTY_ERROR;
> -        *(m_option_t**)arg = prop;
> +        arg_handle = arg;
> +        *arg_handle = prop;

That only obfuscates the code. Also there is no point in defining
arg_handle at the top of the function if it is only used in this block

Greetings,
Reimar Döffinger