[MPlayer-dev-eng] [PATCH] '-priority' support on OS/2

[Diego Biurrun]

On Thu, Feb 05, 2009 at 10:46:21PM +0900, KO Myung-Hun wrote:
> 
> Diego Biurrun wrote:
> >On Thu, Feb 05, 2009 at 02:09:49AM +0900, KO Myung-Hun wrote:
> >  
> >>I attach the updated patch.
> >>
> >>--- Makefile	(revision 28306)
> >>+++ Makefile	(working copy)
> >>@@ -433,6 +433,7 @@
> >>
> >> SRCS_COMMON-$(PNG) 				  += libmpcodecs/vd_mpng.c
> >>+SRCS_COMMON-$(PRIORITY)				 += 
> >>osdep/$(PRIORITY_C)
> >> SRCS_COMMON-$(PVR) 				  += stream/stream_pvr.c
> >> SRCS_COMMON-$(QTX_CODECS)			  += 
> >> libmpcodecs/ad_qtaudio.c \
> >
> >Suddenly this file is full of tabs, what happened?
> 
> It's make file, so I compressed spaces to tab as the original. It seems 
> that tab is not matter, therefore I'll not convert spaces to tab.

The original has tabs only in the required places.  As a rule of thumb,
do not mess with whitespace.

> >>--- osdep/priority-win.c    (revision 0)
> >>+++ osdep/priority-win.c    (revision 0)
> >>    
> >>--- osdep/priority-os2.c    (revision 0)
> >>+++ osdep/priority-os2.c    (revision 0)
> >>    
> >
> >I have the impression that this could be one file.
> 
> I made OS-specific files as other osdep files because I thought you 
> didn't like #ifdef. It seems I misunderstood you. Anyway I integrated 
> them to one file using #ifdef.

It depends on the situation.  If a lot of code can be shared, #ifdef can
be cleaner.

> --- Makefile	(revision 28306)
> +++ Makefile    (working copy)
> @@ -215,6 +215,7 @@
>                libvo/osd.c \
>                libvo/sub.c \
>                osdep/$(GETCH) \
> +              osdep/priority.c \

This should be compiled conditional on CONFIG_PRIORITY.

> @@ -721,6 +725,7 @@
>  _def_stream_cache="#define CONFIG_STREAM_CACHE 1"
>  _def_pthread_cache="#undef PTHREAD_CACHE"
>  _need_shmem=yes
> +_def_priority="#undef CONFIG_PRIORITY"

Leave out the leading underscore from the variable name and group all
the definition variables together.

Also, something is wrong with your patch, these hunks are for configure,
not Makefile...

> --- osdep/priority.c	(revision 0)
> +++ osdep/priority.c	(revision 0)
> @@ -0,0 +1,98 @@
> +
> +#include "config.h"
> +
> +#ifdef CONFIG_PRIORITY

Hmmm...  I think it would be cleaner if you would do the call to
set_priority in mplayer.c conditional to CONFIG_PRIORITY instead of
using #ifdef here.

> +    if(proc_priority) {
> +
> +        for(i = 0; priority_presets_defs[i].name; i++) {
> +            if(strcasecmp(priority_presets_defs[i].name, proc_priority) == 0)

Please use K&R style, i.e. space after if/for.

Diego