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

Dan Oscarsson Dan.Oscarsson at tieto.com
Thu May 5 16:12:10 CEST 2011


tor 2011-05-05 klockan 15:20 +0200 skrev Clément Bœsch:
> On Thu, May 05, 2011 at 03:11:47PM +0200, Diego Biurrun 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.
> > > 
> > > Patch updated. No more functionnal changes, and more cosmetics, vertical
> > > align & stuff. It follows the uncrustify patch.
> > > 
> > > --- a/mplayer.c
> > > +++ b/mplayer.c
> > > @@ -449,16 +449,11 @@ char *get_metadata(metadata_t type)
> > >  
> > >      case META_VIDEO_CODEC:
> > > -        if (sh_video->format == 0x10000001)
> > > -            return strdup("mpeg1");
> > > -        else if (sh_video->format == 0x10000002)
> > > -            return strdup("mpeg2");
> > > -        else if (sh_video->format == 0x10000004)
> > > -            return strdup("mpeg4");
> > > -        else if (sh_video->format == 0x10000005)
> > > -            return strdup("h264");
> > > -        else if (sh_video->format >= 0x20202020)
> > > -            return mp_asprintf("%.4s", (char *)&sh_video->format);
> > > +             if (sh_video->format == 0x10000001) return strdup("mpeg1");
> > > +        else if (sh_video->format == 0x10000002) return strdup("mpeg2");
> > > +        else if (sh_video->format == 0x10000004) return strdup("mpeg4");
> > > +        else if (sh_video->format == 0x10000005) return strdup("h264");
> > > +        else if (sh_video->format >= 0x20202020) return mp_asprintf("%.4s", (char *)&sh_video->format);
> > >          return mp_asprintf("0x%08X", sh_video->format);
> > 
> > I prefer the former.  I don't consider it a good idea to stray from the
> > automatic indentation (too much).
> > 
> 
> OK, too bad. I just tought the association was more obvious that way.

I prefer the single line way. Also the way below. Clearer to the eye.

Often, an if statment with a single statement, is also easiest to se if
written on one line.
Indented code is easier to read if indented code has a terminating
symbol (like always use { }) and can easier get lines added/removed.

> 
> > > @@ -479,26 +474,13 @@ char *get_metadata(metadata_t type)
> > >  
> > >      /* check for valid demuxer */
> > > -    case META_INFO_TITLE:
> > > -        return get_demuxer_info("Title");
> > > -
> > > -    case META_INFO_ARTIST:
> > > -        return get_demuxer_info("Artist");
> > > -
> > > -    case META_INFO_ALBUM:
> > > -        return get_demuxer_info("Album");
> > > -
> > > -    case META_INFO_YEAR:
> > > -        return get_demuxer_info("Year");
> > > -
> > > -    case META_INFO_COMMENT:
> > > -        return get_demuxer_info("Comment");
> > > -
> > > -    case META_INFO_TRACK:
> > > -        return get_demuxer_info("Track");
> > > -
> > > -    case META_INFO_GENRE:
> > > -        return get_demuxer_info("Genre");
> > > +    case META_INFO_TITLE:   return get_demuxer_info("Title");
> > > +    case META_INFO_ARTIST:  return get_demuxer_info("Artist");
> > > +    case META_INFO_ALBUM:   return get_demuxer_info("Album");
> > > +    case META_INFO_YEAR:    return get_demuxer_info("Year");
> > > +    case META_INFO_COMMENT: return get_demuxer_info("Comment");
> > > +    case META_INFO_TRACK:   return get_demuxer_info("Track");
> > > +    case META_INFO_GENRE:   return get_demuxer_info("Genre");
> > 
> > same
> > 
> > > @@ -516,14 +498,14 @@ static void print_file_properties(const MPContext *mpctx, const char *filename)
> > >      if (mpctx->sh_video) {
> > >          /* Assume FOURCC if all bytes >= 0x20 (' ') */
> > >          if (mpctx->sh_video->format >= 0x20202020)
> > > -            mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=%.4s\n", (char *)&mpctx->sh_video->format);
> > > +            mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=%.4s\n",   (char *)&mpctx->sh_video->format);
> > >          else
> > >              mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=0x%08X\n", mpctx->sh_video->format);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_BITRATE=%d\n", mpctx->sh_video->i_bps * 8);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_WIDTH=%d\n", mpctx->sh_video->disp_w);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_HEIGHT=%d\n", mpctx->sh_video->disp_h);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FPS=%5.3f\n", mpctx->sh_video->fps);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_ASPECT=%1.4f\n", mpctx->sh_video->aspect);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_VIDEO_BITRATE=%d\n",    mpctx->sh_video->i_bps * 8);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_VIDEO_WIDTH=%d\n",      mpctx->sh_video->disp_w);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_VIDEO_HEIGHT=%d\n",     mpctx->sh_video->disp_h);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_VIDEO_FPS=%5.3f\n",     mpctx->sh_video->fps);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_VIDEO_ASPECT=%1.4f\n",  mpctx->sh_video->aspect);
> > >          video_start_pts = ds_get_next_pts(mpctx->d_video);
> > >      }
> > >      if (mpctx->sh_audio) {
> > > @@ -531,10 +513,10 @@ static void print_file_properties(const MPContext *mpctx, const char *filename)
> > >          if (mpctx->sh_audio->format >= 0x20202020)
> > >              mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%.4s\n", (char *)&mpctx->sh_audio->format);
> > >          else
> > > -            mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%d\n", mpctx->sh_audio->format);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_BITRATE=%d\n", mpctx->sh_audio->i_bps * 8);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_RATE=%d\n", mpctx->sh_audio->samplerate);
> > > -        mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_NCH=%d\n", mpctx->sh_audio->channels);
> > > +            mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%d\n",   mpctx->sh_audio->format);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_AUDIO_BITRATE=%d\n",  mpctx->sh_audio->i_bps * 8);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_AUDIO_RATE=%d\n",     mpctx->sh_audio->samplerate);
> > > +        mp_msg(MSGT_IDENTIFY, MSGL_INFO,     "ID_AUDIO_NCH=%d\n",      mpctx->sh_audio->channels);
> > 
> > I don't see the point.
> > 
> 
> It sticks more to the final output, and the values are aligned to we can
> dicern them more I guess.
> 
> > > @@ -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);
> > >  }
> > 
> > This looks like a bug in uncrustify or the uncrustify configuration.
> > 
> 
> Yep, but I wasn't able to find a correct workaround in the settings. Feel
> free to dig in it…
> 
> > > @@ -1046,14 +1026,11 @@ static int playtree_add_playlist(play_tree_t *entry)
> > >      {
> > >          if (!entry) {
> > >              entry = mpctx->playtree_iter->tree;
> > > -            if (play_tree_iter_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY) {
> > > +            if (play_tree_iter_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY)
> > >                  return PT_NEXT_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) {
> > > +            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;
> > > -                }
> > > -            }
> > 
> > I think uncrustify has an option for this.
> > 
> 
> Yes under a warning section. We don't set it up because of some issue with
> defines.
> 
> > > @@ -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 {
> > >          if (to_mpi)
> > >              free_mp_image(to_mpi);
> > >          if (from_mpi->w == 0 || from_mpi->h == 0)
> > > @@ -2531,8 +2507,9 @@ static void pause_loop(void)
> > >          if (term_osd && !mpctx->sh_video) {
> > >              set_osd_msg(OSD_MSG_PAUSE, 1, 0, MSGTR_Paused);
> > >              update_osd_msg();
> > > -        } else
> > > +        } else {
> > >              mp_msg(MSGT_CPLAYER, MSGL_STATUS, "\n"MSGTR_Paused "\r");
> > > +        }
> > >          mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_PAUSED\n");
> > >      }
> > >  #ifdef CONFIG_GUI
> > > @@ -2574,9 +2551,10 @@ static void pause_loop(void)
> > >                      set_osd_msg(OSD_MSG_PAUSE, 1, 0, MSGTR_Paused " %d%%",
> > >                                  new_cache_fill);
> > >                      update_osd_msg();
> > > -                } else
> > > +                } else {
> > >                      mp_msg(MSGT_CPLAYER, MSGL_STATUS, MSGTR_Paused " %d%%\r",
> > >                             new_cache_fill);
> > > +                }
> > 
> > I see little point in doing this manually if uncrustify can achieve it.
> > 
> 
> See above.
> 
> > > @@ -3306,19 +3280,15 @@ play_next_file:
> > >  
> > >      if (mpctx->stream->type == STREAMTYPE_BD) {
> > > -        if (audio_lang && audio_id == -1)
> > > -            audio_id = bd_aid_from_lang(mpctx->stream, audio_lang);
> > > -        if (dvdsub_lang && dvdsub_id == -1)
> > > -            dvdsub_id = bd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > > +        if (audio_lang  && audio_id  == -1) audio_id  = bd_aid_from_lang(mpctx->stream, audio_lang);
> > > +        if (dvdsub_lang && dvdsub_id == -1) dvdsub_id = bd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > >      }
> > >  
> > >  #ifdef CONFIG_DVDREAD
> > >      if (mpctx->stream->type == STREAMTYPE_DVD) {
> > >          current_module = "dvd lang->id";
> > > -        if (audio_lang && audio_id == -1)
> > > -            audio_id = dvd_aid_from_lang(mpctx->stream, audio_lang);
> > > -        if (dvdsub_lang && dvdsub_id == -1)
> > > -            dvdsub_id = dvd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > > +        if (audio_lang  && audio_id  == -1) audio_id  = dvd_aid_from_lang(mpctx->stream, audio_lang);
> > > +        if (dvdsub_lang && dvdsub_id == -1) dvdsub_id = dvd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > 
> > I'd skip this as well.
> > 
> 
> OK :(

As I said above - for me it is much clearer when using a single line.


> 
> > > @@ -3878,20 +3828,20 @@ goto_enable_cache:
> > >  
> > >  //============================ Auto QUALITY ============================
> > >  
> > > -/*Output quality adjustments:*/
> > > +                /* Output quality adjustments: */
> > >                  if (auto_quality > 0) {
> > >                      current_module = "autoq";
> > > -//  float total=0.000001f * (GetTimer()-aq_total_time);
> > > -//  if(output_quality<auto_quality && aq_sleep_time>0.05f*total)
> > > +                    //float total=0.000001f * (GetTimer()-aq_total_time);
> > > +                    //if(output_quality<auto_quality && aq_sleep_time>0.05f*total)
> > >                      if (output_quality < auto_quality && aq_sleep_time > 0)
> > >                          ++output_quality;
> > >                      else
> > > -//  if(output_quality>0 && aq_sleep_time<-0.05f*total)
> > > +                    //if(output_quality>0 && aq_sleep_time<-0.05f*total)
> > >                      if (output_quality > 1 && aq_sleep_time < 0)
> > >                          --output_quality;
> > >                      else if (output_quality > 0 && aq_sleep_time < -0.050f) // 50ms
> > >                          output_quality = 0;
> > > -//  printf("total: %8.6f  sleep: %8.6f  q: %d\n",(0.000001f*aq_total_time),aq_sleep_time,output_quality);
> > > +                    //printf("total: %8.6f  sleep: %8.6f  q: %d\n",(0.000001f*aq_total_time),aq_sleep_time,output_quality);
> > >                      set_video_quality(mpctx->sh_video, output_quality);
> > 
> > The style in the commented-out code is messed up :-p
> > 
> 
> oh please :D
> 
> > Most of the rest like the comment adjustments etc. looks OK.
> > 
> 
> OK
> 




More information about the MPlayer-dev-eng mailing list