[Qemu-devel] [PATCH] curses: build with -std=gnu99

[Qemu-devel] [PATCH] curses: build with -std=gnu99

[Gerd Hoffmann]

---
 ui/Makefile.objs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index dc936f1..62f4cf3 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -17,6 +17,9 @@ common-obj-$(CONFIG_CURSES) += curses.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
 
+# needed to make gcc accept wide unicode chars without warning
+curses.o-cflags := -std=gnu99
+
 ifeq ($(CONFIG_SDLABI),1.2)
 sdl.mo-objs := sdl.o sdl_zoom.o
 endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Peter Maydell]

On 27 October 2016 at 12:46, Gerd Hoffmann <kraxel@redhat.com> wrote:
> ---
>  ui/Makefile.objs | 3 +++
>  1 file changed, 3 insertions(+)

Missing commit message and signed-off-by line...

> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index dc936f1..62f4cf3 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -17,6 +17,9 @@ common-obj-$(CONFIG_CURSES) += curses.o
>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>  common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
>
> +# needed to make gcc accept wide unicode chars without warning
> +curses.o-cflags := -std=gnu99
> +
>  ifeq ($(CONFIG_SDLABI),1.2)
>  sdl.mo-objs := sdl.o sdl_zoom.o
>  endif
> --
> 1.8.3.1

I'm not sure about this.

Do we really want gnu99 and not c99? Should we just enable
std=c99 for all source files, given that we already assume
C99 anyway?

It would also be helpful if the commit message quoted the
compiler warning that you get otherwise, so it's easier
to see why we're doing this.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Gerd Hoffmann]

  Hi,

> > +# needed to make gcc accept wide unicode chars without warning
> > +curses.o-cflags := -std=gnu99
> > +
> >  ifeq ($(CONFIG_SDLABI),1.2)
> >  sdl.mo-objs := sdl.o sdl_zoom.o
> >  endif
> > --
> > 1.8.3.1
> 
> I'm not sure about this.
> 
> Do we really want gnu99 and not c99? Should we just enable
> std=c99 for all source files, given that we already assume
> C99 anyway?

-std=c99 doesn't fly, throws errors on (gcc-specific) asm.
So we have to use -std=gnu99.

Tried to do that tree-wide by patching configure but got build errors
then (somewhere in iscsi code).

> It would also be helpful if the commit message quoted the
> compiler warning that you get otherwise, so it's easier
> to see why we're doing this.

With the wide char curses patches by samuel applied I get errors like
this one:

/home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
character names are only valid in C++ and C99 [-Werror]
             case L'\u23bd':
                  ^

I'm open to better ideas to handle this.

Failing that I can cut+paste this reply into the commit message to
clarify things.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Samuel Thibault]

Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
> character names are only valid in C++ and C99 [-Werror]
>              case L'\u23bd':

Another way could be to assume unicode encoding of wchar_t characters
(which looks very reasonable to me) and just write "case 0x23bd:".

Samuel

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Peter Maydell]

On 27 October 2016 at 16:58, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
>> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
>> character names are only valid in C++ and C99 [-Werror]
>>              case L'\u23bd':
>
> Another way could be to assume unicode encoding of wchar_t characters
> (which looks very reasonable to me) and just write "case 0x23bd:".

Does this still work if you're using curses on mingw32?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Samuel Thibault]

Peter Maydell, on Thu 27 Oct 2016 17:14:52 +0100, wrote:
> On 27 October 2016 at 16:58, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
> >> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
> >> character names are only valid in C++ and C99 [-Werror]
> >>              case L'\u23bd':
> >
> > Another way could be to assume unicode encoding of wchar_t characters
> > (which looks very reasonable to me) and just write "case 0x23bd:".
> 
> Does this still work if you're using curses on mingw32?

Windows' wchar_t uses unicode encoding, yes (and its limitation to 16bit
doesn't pose problem to the values we care about).

Samuel

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Peter Maydell]

On 27 October 2016 at 17:36, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Peter Maydell, on Thu 27 Oct 2016 17:14:52 +0100, wrote:
>> On 27 October 2016 at 16:58, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>> > Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
>> >> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
>> >> character names are only valid in C++ and C99 [-Werror]
>> >>              case L'\u23bd':
>> >
>> > Another way could be to assume unicode encoding of wchar_t characters
>> > (which looks very reasonable to me) and just write "case 0x23bd:".
>>
>> Does this still work if you're using curses on mingw32?
>
> Windows' wchar_t uses unicode encoding, yes (and its limitation to 16bit
> doesn't pose problem to the values we care about).

On the other hand apparently FreeBSD and Solaris have a wchar_t
whose encoding is locale-dependent:

http://www.thecodingforums.com/threads/wchar_t-is-useless.806149/#post-4398211

thanks
-- PMM

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Samuel Thibault]

Peter Maydell, on Thu 27 Oct 2016 17:52:17 +0100, wrote:
> On 27 October 2016 at 17:36, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Peter Maydell, on Thu 27 Oct 2016 17:14:52 +0100, wrote:
> >> On 27 October 2016 at 16:58, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> >> > Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
> >> >> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
> >> >> character names are only valid in C++ and C99 [-Werror]
> >> >>              case L'\u23bd':
> >> >
> >> > Another way could be to assume unicode encoding of wchar_t characters
> >> > (which looks very reasonable to me) and just write "case 0x23bd:".
> >>
> >> Does this still work if you're using curses on mingw32?
> >
> > Windows' wchar_t uses unicode encoding, yes (and its limitation to 16bit
> > doesn't pose problem to the values we care about).
> 
> On the other hand apparently FreeBSD and Solaris have a wchar_t
> whose encoding is locale-dependent:
> 
> http://www.thecodingforums.com/threads/wchar_t-is-useless.806149/#post-4398211

UURrgll... So we can't use L'\u23bd' on such systems, it would just not
work either, we have to use iconv to get these right...

Samuel

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Markus Armbruster]

Samuel Thibault <samuel.thibault@gnu.org> writes:

> Peter Maydell, on Thu 27 Oct 2016 17:52:17 +0100, wrote:
>> On 27 October 2016 at 17:36, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>> > Peter Maydell, on Thu 27 Oct 2016 17:14:52 +0100, wrote:
>> >> On 27 October 2016 at 16:58, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>> >> > Gerd Hoffmann, on Thu 27 Oct 2016 17:55:47 +0200, wrote:
>> >> >> /home/kraxel/projects/qemu/ui/curses.c:627:18: error: universal
>> >> >> character names are only valid in C++ and C99 [-Werror]
>> >> >>              case L'\u23bd':
>> >> >
>> >> > Another way could be to assume unicode encoding of wchar_t characters
>> >> > (which looks very reasonable to me) and just write "case 0x23bd:".
>> >>
>> >> Does this still work if you're using curses on mingw32?
>> >
>> > Windows' wchar_t uses unicode encoding, yes (and its limitation to 16bit
>> > doesn't pose problem to the values we care about).
>> 
>> On the other hand apparently FreeBSD and Solaris have a wchar_t
>> whose encoding is locale-dependent:
>> 
>> http://www.thecodingforums.com/threads/wchar_t-is-useless.806149/#post-4398211
>
> UURrgll... So we can't use L'\u23bd' on such systems, it would just not
> work either, we have to use iconv to get these right...

I guess we can if they actually bother to conform to C99.  6.4.3
Universal character names:

    The universal character name \Unnnnnnnn designates the character
    whose eight-digit short identifier (as specified by ISO/IEC 10646)
    is nnnnnnnn.  Similarly, the universal character name \unnnn
    designates the character whose four-digit short identifier is nnnn
    (and whose eight-digit short identifier is 0000nnnn).

Note that it doesn't say here that L'\u23bd' should be equal to 0x23bd.

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Samuel Thibault]

Markus Armbruster, on Fri 28 Oct 2016 08:51:20 +0200, wrote:
> Samuel Thibault <samuel.thibault@gnu.org> writes:
> 
> > Peter Maydell, on Thu 27 Oct 2016 17:52:17 +0100, wrote:
> >> http://www.thecodingforums.com/threads/wchar_t-is-useless.806149/#post-4398211
> >
> > UURrgll... So we can't use L'\u23bd' on such systems, it would just not
> > work either, we have to use iconv to get these right...
> 
> I guess we can if they actually bother to conform to C99.  6.4.3
> Universal character names:
> 
>     The universal character name \Unnnnnnnn designates the character
>     whose eight-digit short identifier (as specified by ISO/IEC 10646)
>     is nnnnnnnn.  Similarly, the universal character name \unnnn
>     designates the character whose four-digit short identifier is nnnn
>     (and whose eight-digit short identifier is 0000nnnn).
> 
> Note that it doesn't say here that L'\u23bd' should be equal to 0x23bd.

I know, but as discussed in the thread, that could only work if gcc
embeds something like calls to nl_langinfo and iconv to properly convert
from the unicode value to the proper wchar_t according to the locale.  I
wouldn't be surprised that they don't.

Samuel

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Markus Armbruster]

Samuel Thibault <samuel.thibault@gnu.org> writes:

> Markus Armbruster, on Fri 28 Oct 2016 08:51:20 +0200, wrote:
>> Samuel Thibault <samuel.thibault@gnu.org> writes:
>> 
>> > Peter Maydell, on Thu 27 Oct 2016 17:52:17 +0100, wrote:
>> >> http://www.thecodingforums.com/threads/wchar_t-is-useless.806149/#post-4398211
>> >
>> > UURrgll... So we can't use L'\u23bd' on such systems, it would just not
>> > work either, we have to use iconv to get these right...
>> 
>> I guess we can if they actually bother to conform to C99.  6.4.3
>> Universal character names:
>> 
>>     The universal character name \Unnnnnnnn designates the character
>>     whose eight-digit short identifier (as specified by ISO/IEC 10646)
>>     is nnnnnnnn.  Similarly, the universal character name \unnnn
>>     designates the character whose four-digit short identifier is nnnn
>>     (and whose eight-digit short identifier is 0000nnnn).
>> 
>> Note that it doesn't say here that L'\u23bd' should be equal to 0x23bd.
>
> I know, but as discussed in the thread, that could only work if gcc
> embeds something like calls to nl_langinfo and iconv to properly convert
> from the unicode value to the proper wchar_t according to the locale.  I
> wouldn't be surprised that they don't.

Before we uglify our code to work around (egregious!) defects in certain
compilers, we should find out whether and where exactly these defects
exist, and whether it's worth targeting the defective compilers.

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Gerd Hoffmann]

  Hi,

> Before we uglify our code to work around (egregious!) defects in certain
> compilers, we should find out whether and where exactly these defects
> exist, and whether it's worth targeting the defective compilers.

Using iconv is on the table _anyway_, to properly support
"-display curses,charset=$something", so using that for the cp437 table
too actually simplifies things and avoids hard-coding tables using
\uxxxx as side effect.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[G 3]

On Oct 27, 2016, at 11:07 AM, Gerd Hoffmann wrote:

>>> +# needed to make gcc accept wide unicode chars without warning
>>> +curses.o-cflags := -std=gnu99
>
>> Could we add a commit message to this patch? It could answer
>> questions like:
>>
>> Why this patch is needed?
>> Who needs this patch?
>> What problem it solves.
>
> The comment added by the patch explains things.
>
> I could cut+paste that into the commit msg, but given how small the
> patch is it looked pointless to me ...
>
> cheers,
>   Gerd

A commit message should go with every patch. It makes it easier for  
someone looking thru the commit history to understand your patch.

This might be good enough:

Makes gcc accept wide unicode characters without producing a warning  
during compilation.

Re: [Qemu-devel] [PATCH] curses: build with -std=gnu99

[Gerd Hoffmann]

> > +# needed to make gcc accept wide unicode chars without warning
> > +curses.o-cflags := -std=gnu99

> Could we add a commit message to this patch? It could answer  
> questions like:
> 
> Why this patch is needed?
> Who needs this patch?
> What problem it solves.

The comment added by the patch explains things.

I could cut+paste that into the commit msg, but given how small the
patch is it looked pointless to me ...

cheers,
  Gerd

[Qemu-devel] [PATCH] curses: build with -std=gnu99

[G 3]

On Oct 27, 2016, at 8:04 AM, qemu-devel-request@nongnu.org wrote:

> ---
>  ui/Makefile.objs | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index dc936f1..62f4cf3 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -17,6 +17,9 @@ common-obj-$(CONFIG_CURSES) += curses.o
>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>  common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
>
> +# needed to make gcc accept wide unicode chars without warning
> +curses.o-cflags := -std=gnu99
> +
>  ifeq ($(CONFIG_SDLABI),1.2)
>  sdl.mo-objs := sdl.o sdl_zoom.o
>  endif
> --  
> 1.8.3.1

Could we add a commit message to this patch? It could answer  
questions like:

Why this patch is needed?
Who needs this patch?
What problem it solves.