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

[Clément Bœsch]

On Thu, May 05, 2011 at 06:49:58PM +0200, Reimar Döffinger wrote:
> 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.
> 

I wouldn't do it in a bunch of 10 cosmetic commits, but since we're at it…

> > @@ -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);
> 

I prefer that later form. Chunk dropped, I'll fix that this way.

> > -            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;
> 

OK, fixed locally.

> > @@ -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.
> 

Just for consistency with the whole if/else.

-- 
Clément B.