[MPlayer-cvslog] r35472 - trunk/gui/util/list.c

[Alexander Strasser]

Hi Ingo,

ib wrote:
> Author: ib
> Date: Sun Nov 25 13:24:02 2012
> New Revision: 35472
> 
> Log:
> Improve add_to_gui_playlist().
> 
> Use pointer arithmetic rather than strlen() and rename variables.
                                                  ^^^^^^

  I would advise to do the renaming in a separate commit. It makes
reading the diffs unnecessarily harder.

> Moreover, a non-existing path (although not supposed to happen)
> shouldn't be an empty string.
> 
> Modified:
>    trunk/gui/util/list.c
> 
> Modified: trunk/gui/util/list.c
> ==============================================================================
> --- trunk/gui/util/list.c	Sun Nov 25 12:55:07 2012	(r35471)
> +++ trunk/gui/util/list.c	Sun Nov 25 13:24:02 2012	(r35472)
> @@ -289,29 +289,30 @@ void listRepl(char ***list, const char *
>   */
>  int add_to_gui_playlist(const char *what, int how)
>  {
> -    char *filename, *pathname;
> +    const char *file;
> +    char *path;
>      plItem *item;
>  
>      if (!what || (how != PLAYLIST_ITEM_APPEND && how != PLAYLIST_ITEM_INSERT))
>          return 0;

  Would make sense to add a "*what != 0" in the middle. Read on below
to see why I think it is necessary with the new code.

> -    filename = strdup(mp_basename(what));
> -    pathname = strdup(what);
> +    file = mp_basename(what);
> +    path = strdup(what);
>  
> -    if (strlen(pathname) - strlen(filename) > 0)
> -        pathname[strlen(pathname) - strlen(filename) - 1] = 0;                                            // we have some path, so remove / at end
> +    if (file > what)
> +        path[file - what - 1] = 0;
>      else
> -        pathname[strlen(pathname) - strlen(filename)] = 0;
> +        strcpy(path, ".");

  This is writing one past the strdup'd buffer if the parameter what
is an empty string.

>      item = calloc(1, sizeof(plItem));
>  
>      if (!item)
>          return 0;
>  
> -    mp_msg(MSGT_GPLAYER, MSGL_DBG2, "[list] adding %s/%s\n", pathname, filename);
> +    mp_msg(MSGT_GPLAYER, MSGL_DBG2, "[list] adding %s/%s\n", path, file);
>  
> -    item->name = filename;
> -    item->path = pathname;
> +    item->name = strdup(file);
                    ^^^^^^
> +    item->path = path;

  Closing comment: Maybe not switching to doing the pointer comparison
and factorizing the strlen calls would result in more straight forward
code. I think the new code is harder to read because of the asymmetric
string duplication.

  Alexander