Re: [patch] fix phony_iconv wide character support

[Pedro Alves <palves@redhat.com>]

On 12/31/2015 04:48 PM, Sandra Loosemore wrote:

> 
> diff --git a/gdb/charset.c b/gdb/charset.c
> index ee1ae20..82e5644 100644
> --- a/gdb/charset.c
> +++ b/gdb/charset.c
> @@ -77,9 +77,13 @@
>     arrange for there to be a single available character set.  */
>  
>  #undef GDB_DEFAULT_HOST_CHARSET
> +#ifdef USE_WIN32API
> +#define GDB_DEFAULT_HOST_CHARSET "CP1252"
> +#else
>  #define GDB_DEFAULT_HOST_CHARSET "ISO-8859-1"
> -#define GDB_DEFAULT_TARGET_CHARSET "ISO-8859-1"
> -#define GDB_DEFAULT_TARGET_WIDE_CHARSET "ISO-8859-1"
> +#endif

Could you indent this, like:

#ifdef USE_WIN32API
# define GDB_DEFAULT_HOST_CHARSET "CP1252"
#else
# define GDB_DEFAULT_HOST_CHARSET "ISO-8859-1"
#endif


> +#define GDB_DEFAULT_TARGET_CHARSET GDB_DEFAULT_HOST_CHARSET 
> +#define GDB_DEFAULT_TARGET_WIDE_CHARSET "UTF-32"
>  #undef DEFAULT_CHARSET_NAMES
>  #define DEFAULT_CHARSET_NAMES GDB_DEFAULT_HOST_CHARSET ,
>  
> @@ -95,20 +99,27 @@
>  #undef ICONV_CONST
>  #define ICONV_CONST const
>  
> +/* We allow conversions from UTF-32, wchar_t, and the host charset.
> +   We allow conversions to wchar_t and the host charset

Missing period.

> +   Return 1 if we are converting from UTF-32BE, 2 if from UTF32-LE,
> +   0 otherwise.  This is  used as a flag in calls to iconv.  */

Spurious double space after "This is".


> +
> +  return 0;
>  }
>  
>  static int
> @@ -130,8 +141,17 @@ phony_iconv (iconv_t utf_flag, const char **inbuf, size_t *inbytesleft,
>  
>  	  for (j = 0; j < 4; ++j)
>  	    {
> -	      c <<= 8;
> -	      c += (*inbuf)[j] & 0xff;
> +	      if (utf_flag == 1)
> +		{
> +		  /* Big-endian.  */
> +		  c <<= 8;
> +		  c += (*inbuf)[j] & 0xff;
> +		}
> +	      else
> +		{
> +		  /* Little-endian.  */
> +		  c += ((*inbuf)[j] & 0xff) << (8 * j);
> +		}
>  	    }


Isn't this the same as:

   enum bfd_endian endian = utf_flag == 1 ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
   extract_unsigned_integer (*inbuf, 4, endian);

?

>  
>    /* The number of non-reversible conversions -- but they were all
> @@ -290,6 +315,10 @@ set_be_le_names (struct gdbarch *gdbarch)
>      return;
>    be_le_arch = gdbarch;
>  
> +#ifdef PHONY_ICONV
> +  target_wide_charset_le_name = "UTF-32LE";
> +  target_wide_charset_be_name = "UTF-32BE";
> +#else
>    target_wide_charset_le_name = NULL;
>    target_wide_charset_be_name = NULL;
>  
> @@ -313,6 +342,7 @@ set_be_le_names (struct gdbarch *gdbarch)
>  	    target_wide_charset_le_name = charset_enum[i];
>  	}
>      }
> +# endif  /* PHONY_ICONV */

This change isn't obvious to me.  You wrote in the ChangeLog:

> 	(set_be_le_names) [PHONY_ICONV]: Use hard-wired names to match
> 	phony_iconv_open.

But I think this "to match" comment should be here in the sources.

>  }
>  
>  /* 'Set charset', 'set host-charset', 'set target-charset', 'set
> 

Otherwise LGTM.

Thanks,
Pedro Alves