[MPlayer-dev-eng] [PATCH] Mac OS X Finder support

[Reimar Döffinger]

Hi,

> >Umm... are you sure that's a good idea? I think that funtion runs 
> >before
> >mp_msg is initialized... Please double-check. If those are really
> >(fatal) errors, output to stderr is okay I think.
> 
> This should be Ok:
> I've changed back to fprintf(stderr,...) but with a somewhat more 
> informative message, after reporting the error the application will 
> also quit.

That is okay. Only problem with any kind of printf is that it will
hinder working support of piping to stdout (will this ever happen?).

Still a few things I have been thinking about.
Hardly important, but you're not very consistent when it comes to placing spaces (e.g. between function arguments) ;-).

> +               Boolean valid;

MPlayer usually uses int (and 0/1 instead of FALSE/TRUE) because Boolean is not standard C. But that's up to you to decide how you want it.

> +                       argv[0]=NULL;

If you have a simple way to pass it something useful, do it.

> + parm[actualSize]=NULL;

This is confusing. parm[actualSize] is a char, not a pointer. You should
use 0 or '\0' instead of NULL (I personally prefer the former).

>+ argv[argc]=malloc(strlen(url->file));

strlen + 1 I think...

>+ url_unescape_string(argv[argc++],url->file);

Although it is less to write, using constructs with argc++ in the middle
is risky, easily leads to bugs during later changes. Please put it
separately.

> + snprintf(myPsnStr,5+10+1+10+1,"-psn_%u_%u",myPsn.highLongOfPSN,myPsn.lowLongOfPSN);

call me paranoid, but I'd actually prefer something like
snprintf(myPsnStr, 5+10+1+10, ...);
myPsnStr[5+10+1+10] = 0;

you forgot to remove the mp_msg.h include.

I think it is so far okay with me, testing (and thus applying) must be done by someone else
though, I don't have a Mac.

Greetings,
Reimar Döffinger