[MPlayer-dev-eng] ctype.h functions on win32

[Rich Felker]

On Sun, Oct 14, 2007 at 06:33:25AM +0200, Gianluigi Tiesi wrote:
> pls try attached patches (splitted mp/lavc/lavf)
> I leaved as is all ints that come from fgetc since
> it's supposed to be ok
> changed chars to unsigned chars where needed
> (and also in other similar functions for conformance)
> cast to uint8_t/uchar(in input.c uint8_t not defined)
> when the source of the value it's not so clear
> (at least for me of course :D)
> 
> Bye
> 
> -- 
> Gianluigi Tiesi <sherpya at netfarm.it>
> EDP Project Leader
> Netfarm S.r.l. - http://www.netfarm.it/
> Free Software: http://oss.netfarm.it/

> Index: vobsub.c
> ===================================================================
> --- vobsub.c	(revision 24775)
> +++ vobsub.c	(working copy)
> @@ -225,7 +225,7 @@
>  /**********************************************************************/
>  
>  static ssize_t
> -vobsub_getline(char **lineptr, size_t *n, rar_stream_t *stream)
> +vobsub_getline(unsigned char **lineptr, size_t *n, rar_stream_t *stream)

OK.

> -	char *tmp = realloc(*lineptr, 4096);
> +	unsigned char *tmp = (unsigned char *) realloc(*lineptr, 4096);

Never cast return values of malloc/realloc.

>  static int
> -vobsub_parse_id(vobsub_t *vob, const char *line)
> +vobsub_parse_id(vobsub_t *vob, const unsigned char *line)
>  {
>      // id: xx, index: n
>      size_t idlen;
> -    const char *p, *q;
> +    const unsigned char *p, *q;
>      p  = line;
>      while (isspace(*p))

Looks ok.

>  static int
> -vobsub_parse_timestamp(vobsub_t *vob, const char *line)
> +vobsub_parse_timestamp(vobsub_t *vob, const unsigned char *line)
>  {
>      // timestamp: HH:MM:SS.mmm, filepos: 0nnnnnnnnn
> -    const char *p;
> +    unsigned const char *p;
>      int h, m, s, ms;
>      off_t filepos;
>      while (isspace(*line))

Looks ok.

>  static int
> -vobsub_parse_size(vobsub_t *vob, const char *line)
> +vobsub_parse_size(vobsub_t *vob, const unsigned char *line)
>  {
>      // size: WWWxHHH
>      char *p;
> @@ -782,7 +782,7 @@
>  }
>  
>  static int
> -vobsub_parse_origin(vobsub_t *vob, const char *line)
> +vobsub_parse_origin(vobsub_t *vob, const unsigned char *line)

Ditto.

>  {
>      // org: X,Y
>      char *p;

Is this p used incorrectly?

>  static int
> -vobsub_parse_palette(vobsub_t *vob, const char *line)
> +vobsub_parse_palette(vobsub_t *vob, const unsigned char *line)
>  {
>      // palette: XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX, XXXXXX
>      unsigned int n;
>      n = 0;
>      while (1) {
> -	const char *p;
> +	const unsigned char *p;
>  	int r, g, b, y, u, v, tmp;
>  	while (isspace(*line))

Looks ok.

> Index: stream/udp.c
> ===================================================================
> --- stream/udp.c	(revision 24775)
> +++ stream/udp.c	(working copy)
> @@ -69,7 +69,7 @@
>      return -1;
>    }
>  
> -  if (isalpha (url->hostname[0]))
> +  if (isalpha ((unsigned char) url->hostname[0]))
>    {
>  #ifndef HAVE_WINSOCK2
>      hp = (struct hostent *) gethostbyname (url->hostname);
> @@ -120,7 +120,7 @@
>    }
>  	
>  #ifdef HAVE_WINSOCK2
> -  if (isalpha (url->hostname[0]))
> +  if (isalpha ((unsigned char) url->hostname[0]))

OK IMO.

> -int dvd_parse_chapter_range(m_option_t *conf, const char *range) {
> -  const char *s;
> +int dvd_parse_chapter_range(m_option_t *conf, const unsigned char *range) {
> +  const unsigned char *s;
>    char *t;
>    if (!range)
>      return M_OPT_MISSING_PARAM;
> Index: stream/tv.c
> ===================================================================
> --- stream/tv.c	(revision 24775)
> +++ stream/tv.c	(working copy)
> @@ -484,7 +484,7 @@
>  	int channel = 0;
>  	if (tvh->tv_param->channel)
>  	 {
> -	   if (isdigit(*tvh->tv_param->channel))
> +	   if (isdigit((uint8_t) *tvh->tv_param->channel))
>  		/* if tvh->tv_param->channel begins with a digit interpret it as a number */
>  		channel = atoi(tvh->tv_param->channel);

unsigned char is a better name for it.

> Index: codec-cfg.c
> ===================================================================
> --- codec-cfg.c	(revision 24775)
> +++ codec-cfg.c	(working copy)

Probably ok.

> Index: input/input.c
> ===================================================================
> --- input/input.c	(revision 24775)
> +++ input/input.c	(working copy)
> @@ -1353,7 +1353,7 @@
>        return key_names[i].name;
>    }
>    
> -  if(isascii(key)) {
> +  if(isascii((unsigned char) key)) {

isascii is not terribly portable to begin with. Just coding the test
explicitly would make more sense...

> Index: dvdread/ifo_print.c
> ===================================================================
> --- dvdread/ifo_print.c	(revision 24775)
> +++ dvdread/ifo_print.c	(working copy)
> @@ -403,8 +403,8 @@
>    }    
>   
>    if(attr->type == 1) {
> -    if(isalpha((int)(attr->lang_code >> 8))
> -       && isalpha((int)(attr->lang_code & 0xff))) {
> +    if(isalpha((unsigned int)(attr->lang_code >> 8))
> +       && ((unsigned int)(attr->lang_code & 0xff))) {

This change looks nonsensical.

> Index: libvo/gl_common.c
> ===================================================================
> --- libvo/gl_common.c	(revision 24775)
> +++ libvo/gl_common.c	(working copy)
> @@ -389,7 +389,7 @@
>   */
>  int glCreatePPMTex(GLenum target, GLenum fmt, GLint filter,
>                     FILE *f, int *width, int *height, int *maxval) {
> -  unsigned w, h, m, val, bpp;
> +  unsigned w, h, m, bpp;
>    char *data;
>    ppm_skip(f);
>    if (fgetc(f) != 'P' || fgetc(f) != '6')
> @@ -403,8 +403,7 @@
>    ppm_skip(f);
>    if (fscanf(f, "%u", &m) != 1)
>      return 0;
> -  val = fgetc(f);
> -  if (!isspace(val))
> +  if (!isspace(fgetc(f)))

I think this is okay... is isspace guaranteed not to evaluate its
argument more than once?

> -  if(isascii(c)) {
> +  if(isascii((uint8_t) c)) {

if (!(c&0x80))

> -static int eol(char p) {
> +static int eol(unsigned char p) {
>  	return (p=='\r' || p=='\n' || p=='\0');
>  }

Nice for consistency but unnecessary.

>  /* Remove leading and trailing space */
> -static void trail_space(char *s) {
> +static void trail_space(unsigned char *s) {
>  	int i = 0;
>  	while (isspace(s[i])) ++i;
>  	if (i) strcpy(s, s + i);

Good.

> Index: drivers/mga_vid.c
> ===================================================================
> --- drivers/mga_vid.c	(revision 24775)
> +++ drivers/mga_vid.c	(working copy)
> @@ -121,7 +121,7 @@
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,4,0)
>  #include <linux/ctype.h>
>  
> -static unsigned long simple_strtoul(const char *cp,char **endp,unsigned int base)
> +static unsigned long simple_strtoul(const unsigned char *cp,char **endp,unsigned int base)
>  {
>          unsigned long result = 0,value;

Seems unrelated, this is Linux-kernel-specific code using
linux/ctype.h not ctype.h.

> -double ff_eval2(char *s, double *const_value, const char **const_name,
> +double ff_eval2(unsigned char *s, double *const_value, const char **const_name,

Changing api here is unfriendly to the caller. Better would be to cast
the pointer in the implementation of this function if needed.

> -attribute_deprecated double ff_eval(char *s, double *const_value, const char **const_name,
> +attribute_deprecated double ff_eval(unsigned char *s, double *const_value, const char **const_name,

Ditto.

> -int av_get_frame_filename(char *buf, int buf_size,
> +int av_get_frame_filename(unsigned char *buf, int buf_size,

Ditto.

I know I left out a lot of reviewing but hopefully this partial review
is helpful..

Rich