[MPlayer-dev-eng] Export mp_basename in a function

[Clément Bœsch]

On Tue, Nov 16, 2010 at 09:09:57PM +0100, Reimar Döffinger wrote:
> On Fri, Nov 12, 2010 at 09:42:45AM +0100, Clément Bœsch wrote:
> > On Fri, Nov 12, 2010 at 08:14:32AM +0100, Reimar Döffinger wrote:
> > > On Thu, Nov 11, 2010 at 10:57:00PM +0100, Clément Bœsch wrote:
> > > > > Also please use "const char" instead of "char" (I think
> > > > > you might have to change some more code to avoid new warnings,
> > > > > but I think it should be possible without real issues).
> > > > 
> > > > Sure, patch updated. It didn't generate a single warning.
> > > 
> > > The warning would come in when you do it really correctly and
> > > add the const also to the return value, which I'd consider
> > > preferable.
> > 
> > I see. Patch updated. I don't know how to test the libmenu modification,
> > but it builds correctly with --enable-menu.
> > 
> > -- 
> > Clément B.
> > Not sent from a jesusPhone.
> 
> > From 67b1c2cb08eedf4758616e7d6d550cfd5bc6916e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> > Date: Thu, 11 Nov 2010 17:30:36 +0100
> > Subject: [PATCH] Export mp_basename in a function
> > 
> > ---
> >  gui/interface.c   |    2 --
> >  libmenu/menu_pt.c |   17 +++++++++++------
> >  mplayer.c         |   11 ++++-------
> >  path.c            |   13 +++++++++++++
> >  path.h            |    1 +
> >  5 files changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/gui/interface.c b/gui/interface.c
> > index a03281e..a83aaa2 100644
> > --- a/gui/interface.c
> > +++ b/gui/interface.c
> > @@ -1135,8 +1135,6 @@ void * gtkSet( int cmd,float fparam, void * vparam )
> >   return NULL;
> >  }
> >  
> > -#define mp_basename(s) (strrchr(s,'/')==NULL?(char*)s:(strrchr(s,'/')+1))
> > -
> >  #include "playtree.h"
> >  
> >  //This function adds/inserts one file into the gui playlist
> > diff --git a/libmenu/menu_pt.c b/libmenu/menu_pt.c
> > index 80d9cf7..984db1a 100644
> > --- a/libmenu/menu_pt.c
> > +++ b/libmenu/menu_pt.c
> > @@ -24,6 +24,7 @@
> >  #include "config.h"
> >  #include "mp_msg.h"
> >  #include "help_mp.h"
> > +#include "path.h"
> >  
> >  #include "libmpcodecs/img_format.h"
> >  #include "libmpcodecs/mp_image.h"
> > @@ -38,8 +39,6 @@
> >  #include "input/input.h"
> >  #include "access_mpcontext.h"
> >  
> > -#define mp_basename(s) (strrchr((s),'/')==NULL?(char*)(s):(strrchr((s),'/')+1))
> > -
> >  struct list_entry_s {
> >    struct list_entry p;
> >    play_tree_t* pt;
> > @@ -124,8 +123,14 @@ static int read_key(menu_t* menu,int c){
> >    return menu_list_jump_to_key(menu, c);
> >  }
> >  
> > +static void free_entry(list_entry_t* entry)
> > +{
> > +  free(entry->p.txt);
> > +  free(entry);
> > +}
> > +
> >  static void close_menu(menu_t* menu) {
> > -  menu_list_uninit(menu,NULL);
> > +  menu_list_uninit(menu, free_entry);
> >  }
> >  
> >  static int op(menu_t* menu, char* args) {
> > @@ -146,7 +151,7 @@ static int op(menu_t* menu, char* args) {
> >  
> >    if(playtree_iter->tree->parent != playtree_iter->root) {
> >      e = calloc(1,sizeof(list_entry_t));
> > -    e->p.txt = "..";
> > +    e->p.txt = strdup("..");
> >      e->pt = playtree_iter->tree->parent;
> >      menu_list_add_entry(menu,e);
> >    }
> > @@ -156,9 +161,9 @@ static int op(menu_t* menu, char* args) {
> >    for( ; i != NULL ; i = i->next ) {
> >      e = calloc(1,sizeof(list_entry_t));
> >      if(i->files)
> > -      e->p.txt = mp_basename(i->files[0]);
> > +      e->p.txt = strdup(mp_basename(i->files[0]));
> >      else
> > -      e->p.txt = "Group ...";
> > +      e->p.txt = strdup("Group ...");
> >      e->pt = i;
> >      menu_list_add_entry(menu,e);
> >    }
> 
> You shouldn't add strdup()/free(), this mp_basename
> change really isn't supposed to change the behaviour.
> p.txt should be const, and if that generates warnings
> those should probably be suppressed by an explicit
> cast + documentation why it is safe.

Ok then, I'm going to commit it with the cast, but i'd like to make a
second commit to use strdup()/free() for a few reasons:

 - other modules in libmenu allocate and free the .txt fields entries just
   like that
 - I don't get the purpose of making it const and then immediately make
   exceptions, I found the cast solution a bit ugly.
 - it seems there is a leak of the list entry anyway, so a free_entry
   function is needed

Regards,

-- 
Clément B.
Not sent from a jesusPhone.