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

[Chris Welton]

--- Reimar Döffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> 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
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
>
http://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> 

I restored m_property.c and looked at the m_option_t*
in the callback as you suggested. Please look at the
attached patch and tell me if this is better.

I went ahead and checked to make sure that it wasn't
used anywhere else and did a full make clean to double
check.

Thank you again for the advice.

Chris


      ____________________________________________________________________________________
Never miss a thing.  Make Yahoo your home page. 
http://www.yahoo.com/r/hs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: m_property_ctrl_f_to_const_m_option_t.diff
Type: application/octet-stream
Size: 492 bytes
Desc: 2579266344-m_property_ctrl_f_to_const_m_option_t.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071124/46277a62/attachment.obj>