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

[Reimar Döffinger]

On Tue, Nov 16, 2010 at 09:28:59PM +0100, Clément Bœsch wrote:
> On Tue, Nov 16, 2010 at 09:09:57PM +0100, Reimar Döffinger wrote:
> > > @@ -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

That is (I hope) because it's actually necessary for those.

>  - I don't get the purpose of making it const and then immediately make
>    exceptions, I found the cast solution a bit ugly.

Granted, it doesn't make sense to use const here in general.
Using a union might be an option, but it is not very nice either.

>  - it seems there is a leak of the list entry anyway, so a free_entry
>    function is needed

No, uninit calls free() if no free function was given.
Feel free to propose improvements, but I'll probably want to see
valgrind logs that show it actually fixing something - because
the alternative is that I'd have to get familiar with that code
and that's not really something I want to spend my time on.