[MPlayer-dev-eng] [PATCH 5/5] Various rectifications uncrustify wasn't able to do.

[Reimar Döffinger]

On Thu, May 05, 2011 at 03:00:17PM +0200, Clément Bœsch wrote:
> On Sat, Apr 16, 2011 at 12:02:53AM +0200, Clément Bœsch wrote:
> > Due to uncrustify and A.I. limitations, humans are still needed
> > sometimes.
> > ---
> >  mplayer.c |  283 +++++++++++++++++++++++++------------------------------------
> >  1 files changed, 114 insertions(+), 169 deletions(-)
> > 
> 
> Patch updated. No more functionnal changes, and more cosmetics, vertical
> align & stuff. It follows the uncrustify patch.

I think this is rather too much bikeshed material.
The comment changes and removing {} for ifs shoudl be ok, most of the rest I'd
rather drop to avoid wasting time bikeshedding.

> @@ -788,9 +769,8 @@ void exit_player(enum exit_reason how)
>  static void child_sighandler(int x)
>  {
>      pid_t pid;
> -    while ((pid = waitpid(-1, NULL, WNOHANG)) > 0) ;
> +    while ((pid = waitpid(-1, NULL, WNOHANG)) > 0);

I don't think that's an improvement, the sensible
ways of doing it I know are
> +    while ((pid = waitpid(-1, NULL, WNOHANG)) > 0) /* wait */;
or
> +    while ((pid = waitpid(-1, NULL, WNOHANG)) > 0) {}

to clarify it is intended.
Though in this case the obvious way of writing it is
do {
  pid = waitpid(-1, NULL, WNOHANG);
} while (pid > 0);

> -            if (mpctx->playtree_iter->tree == entry) { // Loop with a single file
> -                if (play_tree_iter_up_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY) {
> +            if (mpctx->playtree_iter->tree == entry) // Loop with a single file
> +                if (play_tree_iter_up_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY)
>                      return PT_NEXT_ENTRY;
> -                }
> -            }

Multi-level if without {} are a bit questionable.
Better way:

// Loop with a single file
if (mpctx->playtree_iter->tree == entry &&
    play_tree_iter_up_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY)
    return PT_NEXT_ENTRY;

> @@ -1784,8 +1759,9 @@ static int check_framedrop(double frame_time)
>              ++drop_frame_cnt;
>              ++dropped_frames;
>              return frame_dropping;
> -        } else
> +        } else {
>              dropped_frames = 0;
> +        }

Why? That adds an extra line for no benefit IMO.

> @@ -1927,9 +1903,9 @@ static mp_image_t *mp_dvdnav_copy_mpi(mp_image_t *to_mpi,
>      if (to_mpi &&
>          to_mpi->w == from_mpi->w &&
>          to_mpi->h == from_mpi->h &&
> -        to_mpi->imgfmt == from_mpi->imgfmt)
> +        to_mpi->imgfmt == from_mpi->imgfmt) {
>          mpi = to_mpi;
> -    else {
> +    } else {

In contrast to this, which makes sense.