This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] fix phony_iconv wide character support
- From: Pedro Alves <palves at redhat dot com>
- To: Sandra Loosemore <sandra at codesourcery dot com>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Tue, 12 Jan 2016 14:58:12 +0000
- Subject: Re: [patch] fix phony_iconv wide character support
- Authentication-results: sourceware.org; auth=none
- References: <56855C47 dot 7090004 at codesourcery dot 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