Re: [Spice-devel] bool or gboolean
Hi, On Thu, Feb 16, 2017 at 01:24:04PM +0100, Christophe de Dinechin wrote: > > > On 16 Feb 2017, at 13:17, Pavel Grunt wrote: > > > > On Wed, 2017-02-15 at 11:39 -0500, Frediano Ziglio wrote: > >> Hi, > >> question was raised recently on the ML and IRC. > >> > >> Some time ago we decided to use gboolean but some of us would like > >> to discuss again. > >> > >> As any style changes there's no right or wrong, mainly personal > >> opinions but I think consistency is quite important. > >> > >> Some consideration (feel free to add/remove/comment) > >> - gboolean is more used in the code (about 76%) > >> - TRUE/FALSE are more used (96%) > >> - bool is C99 convention, defined in stdbool.h > >> - using gcc the bool type is a bit different from gboolean > >> (which basically is an int) catching some problems as > >> warnings (like cast between different function pointers > >> using bool instead of gboolean/int) > > this is very important point in my opinion > > > >> - bool is easier to write (OT: and my vim is more happy too) > >> > >> There are 2 kind of decision: > >> - prefer bool or gboolean > >> - stay consistent with constants (bool -> true/false, > >> gboolean -> TRUE/FALSE), continue to use TRUE/FALSE. > >> > >> I think as usual new code should follow style while old > >> for "blame" purposes should stay as is (unless code stop > >> compiling for instance). > > I agree, no mass rename IMHO, moving from int to bool where it fits (teuf's patch) would be great too. > > > >> > >> For opinions > >> > >> - bool/gboolean > >> - bool > >>> +1 > > +1 > +1 +1 > >> > >> - gboolean > >> > >> - consistent with type > >> - yes > >>> +1 > > +1 > +1 +1 > > >> > >> - no > >> > >> Frediano > >> ___ > >> Spice-devel mailing list > >> Spice-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/spice-devel > > ___ > > Spice-devel mailing list > > Spice-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
> On 16 Feb 2017, at 13:17, Pavel Grunt wrote: > > On Wed, 2017-02-15 at 11:39 -0500, Frediano Ziglio wrote: >> Hi, >> question was raised recently on the ML and IRC. >> >> Some time ago we decided to use gboolean but some of us would like >> to discuss again. >> >> As any style changes there's no right or wrong, mainly personal >> opinions but I think consistency is quite important. >> >> Some consideration (feel free to add/remove/comment) >> - gboolean is more used in the code (about 76%) >> - TRUE/FALSE are more used (96%) >> - bool is C99 convention, defined in stdbool.h >> - using gcc the bool type is a bit different from gboolean >> (which basically is an int) catching some problems as >> warnings (like cast between different function pointers >> using bool instead of gboolean/int) > this is very important point in my opinion > >> - bool is easier to write (OT: and my vim is more happy too) >> >> There are 2 kind of decision: >> - prefer bool or gboolean >> - stay consistent with constants (bool -> true/false, >> gboolean -> TRUE/FALSE), continue to use TRUE/FALSE. >> >> I think as usual new code should follow style while old >> for "blame" purposes should stay as is (unless code stop >> compiling for instance). > I agree, no mass rename > >> >> For opinions >> >> - bool/gboolean >> - bool >>> +1 > +1 +1 >> >> - gboolean >> >> - consistent with type >> - yes >>> +1 > +1 +1 >> >> - no >> >> Frediano >> ___ >> Spice-devel mailing list >> Spice-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/spice-devel > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
>
> On Wed, Feb 15, 2017 at 04:59:43PM +, Daniel P. Berrange wrote:
> > On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> > > Hi,
> > > question was raised recently on the ML and IRC.
> > >
> > > Some time ago we decided to use gboolean but some of us would like
> > > to discuss again.
> > >
> > > As any style changes there's no right or wrong, mainly personal
> > > opinions but I think consistency is quite important.
> > >
> > > Some consideration (feel free to add/remove/comment)
> > > - gboolean is more used in the code (about 76%)
> > > - TRUE/FALSE are more used (96%)
> > > - bool is C99 convention, defined in stdbool.h
> > > - using gcc the bool type is a bit different from gboolean
> > > (which basically is an int) catching some problems as
> > > warnings (like cast between different function pointers
> > > using bool instead of gboolean/int)
> > > - bool is easier to write (OT: and my vim is more happy too)
> >
> > As noted above gboolean & bool are different types, in fact they
> > are different sizes too (4 bytes vs 1 byte).
> >
> > If you're using glib2, you'll find various places where it wants
> > callbacks which have a gboolean return value, or parameters. You
> > can't pass in a callback which uses bool, as that's not type
> > compatible. So no matter what, you'll always need to use gboolean
> > in some portion of the code, even if you would rather have bool.
>
> NB: There is one place in the code with such a constraint already, which
> is the interfacing with libpjeg:
>
> static boolean dest_mgr_empty_output_buffer(j_compress_ptr cinfo)
> static boolean empty_mem_output_buffer(j_compress_ptr cinfo)
>
> this 'boolean' is equivalent to an int, and can't be changed to bool.
>
> Christophe
>
I also found that spice-common uses some bool.
Maybe adding a new spice_bool type (clearly a joke)!
Sometimes I miss Byte and Boolean from the old good Pascal.
I still prefer bool, true and false, just apparently we can't get
rid of all boolean type definitions, like this code:
strchr("foo", 'f')[0] = 1;
will compile perfectly and crash in C (this will fail to even
compile with C++).
Some weeks ago I found some K&R C style declaration!
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
On Wed, 2017-02-15 at 11:39 -0500, Frediano Ziglio wrote: > Hi, > question was raised recently on the ML and IRC. > > Some time ago we decided to use gboolean but some of us would like > to discuss again. > > As any style changes there's no right or wrong, mainly personal > opinions but I think consistency is quite important. > > Some consideration (feel free to add/remove/comment) > - gboolean is more used in the code (about 76%) > - TRUE/FALSE are more used (96%) > - bool is C99 convention, defined in stdbool.h > - using gcc the bool type is a bit different from gboolean > (which basically is an int) catching some problems as > warnings (like cast between different function pointers > using bool instead of gboolean/int) this is very important point in my opinion > - bool is easier to write (OT: and my vim is more happy too) > > There are 2 kind of decision: > - prefer bool or gboolean > - stay consistent with constants (bool -> true/false, > gboolean -> TRUE/FALSE), continue to use TRUE/FALSE. > > I think as usual new code should follow style while old > for "blame" purposes should stay as is (unless code stop > compiling for instance). I agree, no mass rename > > For opinions > > - bool/gboolean > - bool > > +1 +1 > > - gboolean > > - consistent with type > - yes > > +1 +1 > > - no > > Frediano > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
On Wed, Feb 15, 2017 at 04:59:43PM +, Daniel P. Berrange wrote: > On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote: > > Hi, > > question was raised recently on the ML and IRC. > > > > Some time ago we decided to use gboolean but some of us would like > > to discuss again. > > > > As any style changes there's no right or wrong, mainly personal > > opinions but I think consistency is quite important. > > > > Some consideration (feel free to add/remove/comment) > > - gboolean is more used in the code (about 76%) > > - TRUE/FALSE are more used (96%) > > - bool is C99 convention, defined in stdbool.h > > - using gcc the bool type is a bit different from gboolean > > (which basically is an int) catching some problems as > > warnings (like cast between different function pointers > > using bool instead of gboolean/int) > > - bool is easier to write (OT: and my vim is more happy too) > > As noted above gboolean & bool are different types, in fact they > are different sizes too (4 bytes vs 1 byte). > > If you're using glib2, you'll find various places where it wants > callbacks which have a gboolean return value, or parameters. You > can't pass in a callback which uses bool, as that's not type > compatible. So no matter what, you'll always need to use gboolean > in some portion of the code, even if you would rather have bool. NB: There is one place in the code with such a constraint already, which is the interfacing with libpjeg: static boolean dest_mgr_empty_output_buffer(j_compress_ptr cinfo) static boolean empty_mem_output_buffer(j_compress_ptr cinfo) this 'boolean' is equivalent to an int, and can't be changed to bool. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
>
> On Wed, 2017-02-15 at 18:01 +0100, Christophe Fergeau wrote:
> > On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> > > Hi,
> > > question was raised recently on the ML and IRC.
> > >
> > > Some time ago we decided to use gboolean but some of us would like
> > > to discuss again.
> >
> > So... As said a few times, I would have preferred to wait for a bit
> > to
> > have that discussion as I have a concrete patch which it would go
> > well
> > with...
> >
> > This started with Jonathon's comment in
> > https://lists.freedesktop.org/archives/spice-devel/2017-February/0355
> > 79.html
> > where I returned -1 from a function which should be returning a
> > boolean.
> > The function was declared as 'int foo()', so it was not clear from
> > its
> > prototype that it's returning a bool, and I had totally missed that.
> > So I decided to go ahead and grep'ed for 'return (TRUE|FALSE)' over
> > the
> > code base, and make sure all these functions don't return int.
> >
> >
> > I started with 'gboolean', but then realized that using 'bool', the
> > compiler would be able to help me.
> >
> > static int foo(void);
> >
> > static gboolean foo(void)
> > { }
> >
> > does not raise warnings while
> >
> > static int foo(void);
> >
> > static bool foo(void)
> > { }
> >
> > does raise a warning, and it's the same with function pointers (see
> > attached bool.c file for a test case)
> >
> >
> > So after realizing that, I'd favour bool over gboolean, at least in
> > function prototypes, as it gives us slightly stronger typing.
>
> That's a pretty strong argument for bool, in my opinion. In general,
> I'm in favor of increased type-safety. Daniel's point about glib
> requiring gboolean for callbacks etc is valid, but I'm curious about
> how many cases there would be. It looks like the patch that you
> attached only attempts to change int types to bool, but doesn't touch
> any existing gboolean types, right? It would be interesting to try to
> convert gboolean to bool to see how many of them can be easily changed.
>
I did the check
./event-loop.c:static gboolean timer_func(gpointer user_data)
./tests/replay.c:static gboolean print_count = FALSE;
./tests/replay.c:static gboolean fill_queue_idle(gpointer user_data)
./tests/replay.c:static gboolean progress_timer(gpointer user_data)
./tests/replay.c:gboolean wait = FALSE;
./tests/test-gst.c:static gboolean top_down = FALSE;
./tests/test-gst.c:gboolean use_hw_encoder = FALSE; // TODO use
./red-worker.c:static gboolean worker_source_prepare(GSource *source, gint
*p_timeout)
./red-worker.c:static gboolean worker_source_check(GSource *source)
./red-worker.c:static gboolean worker_source_dispatch(GSource *source,
GSourceFunc callback,
./red-channel-client.c:static gboolean
red_channel_client_initable_init(GInitable *initable,
./red-channel-client.c:static gboolean
red_channel_client_initable_init(GInitable *initable,
>
> >
> > Regarding TRUE vs true, I don't really care, I'd stick with TRUE as
> > that
> > means less churn on the codebase, but I agree it's not really nice.
>
> After you told me that trick with tig blame, I'm not quite as concerned
> about code churn as I used to be ;) A third option would be to
> standardize on 'true/false' for new code and slowly change existing
> TRUE/FALSE as we modify code...
>
> Jonathon
>
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
On Wed, 2017-02-15 at 18:01 +0100, Christophe Fergeau wrote:
> On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> > Hi,
> > question was raised recently on the ML and IRC.
> >
> > Some time ago we decided to use gboolean but some of us would like
> > to discuss again.
>
> So... As said a few times, I would have preferred to wait for a bit
> to
> have that discussion as I have a concrete patch which it would go
> well
> with...
>
> This started with Jonathon's comment in
> https://lists.freedesktop.org/archives/spice-devel/2017-February/0355
> 79.html
> where I returned -1 from a function which should be returning a
> boolean.
> The function was declared as 'int foo()', so it was not clear from
> its
> prototype that it's returning a bool, and I had totally missed that.
> So I decided to go ahead and grep'ed for 'return (TRUE|FALSE)' over
> the
> code base, and make sure all these functions don't return int.
>
>
> I started with 'gboolean', but then realized that using 'bool', the
> compiler would be able to help me.
>
> static int foo(void);
>
> static gboolean foo(void)
> { }
>
> does not raise warnings while
>
> static int foo(void);
>
> static bool foo(void)
> { }
>
> does raise a warning, and it's the same with function pointers (see
> attached bool.c file for a test case)
>
>
> So after realizing that, I'd favour bool over gboolean, at least in
> function prototypes, as it gives us slightly stronger typing.
That's a pretty strong argument for bool, in my opinion. In general,
I'm in favor of increased type-safety. Daniel's point about glib
requiring gboolean for callbacks etc is valid, but I'm curious about
how many cases there would be. It looks like the patch that you
attached only attempts to change int types to bool, but doesn't touch
any existing gboolean types, right? It would be interesting to try to
convert gboolean to bool to see how many of them can be easily changed.
>
> Regarding TRUE vs true, I don't really care, I'd stick with TRUE as
> that
> means less churn on the codebase, but I agree it's not really nice.
After you told me that trick with tig blame, I'm not quite as concerned
about code churn as I used to be ;) A third option would be to
standardize on 'true/false' for new code and slowly change existing
TRUE/FALSE as we modify code...
Jonathon
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] bool or gboolean
On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> Hi,
> question was raised recently on the ML and IRC.
>
> Some time ago we decided to use gboolean but some of us would like
> to discuss again.
So... As said a few times, I would have preferred to wait for a bit to
have that discussion as I have a concrete patch which it would go well
with...
This started with Jonathon's comment in
https://lists.freedesktop.org/archives/spice-devel/2017-February/035579.html
where I returned -1 from a function which should be returning a boolean.
The function was declared as 'int foo()', so it was not clear from its
prototype that it's returning a bool, and I had totally missed that.
So I decided to go ahead and grep'ed for 'return (TRUE|FALSE)' over the
code base, and make sure all these functions don't return int.
I started with 'gboolean', but then realized that using 'bool', the
compiler would be able to help me.
static int foo(void);
static gboolean foo(void)
{ }
does not raise warnings while
static int foo(void);
static bool foo(void)
{ }
does raise a warning, and it's the same with function pointers (see
attached bool.c file for a test case)
So after realizing that, I'd favour bool over gboolean, at least in
function prototypes, as it gives us slightly stronger typing.
Regarding TRUE vs true, I don't really care, I'd stick with TRUE as that
means less churn on the codebase, but I agree it's not really nice.
In addition to the testcase, I'm attaching that s/int/bool patch even
though this version is not going to apply on git master, and is not
tested.
Christophe
From db86accb52c56067fdf2707d53c7ecb4570db446 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau
Date: Mon, 13 Feb 2017 14:55:08 +0100
Subject: [spice-server] s/int/bool
---
server/char-device.c | 20 ++--
server/char-device.h | 18 +-
server/common-graphics-channel.c | 2 +-
server/common-graphics-channel.h | 2 +-
server/dcc-send.c | 36 +--
server/dcc.c | 40 +++
server/dcc.h | 12 ++--
server/display-channel.c | 16
server/display-channel.h | 2 +-
server/glz-encoder-dict.c | 4 ++--
server/glz-encoder.c | 2 +-
server/image-cache.c | 2 +-
server/image-encoders.c | 30 ++---
server/image-encoders.h | 28 +--
server/inputs-channel.c | 12 ++--
server/main-channel-client.c | 4 ++--
server/main-channel.c | 10 +-
server/mjpeg-encoder.c| 4 ++--
server/pixmap-cache.c | 2 +-
server/pixmap-cache.h | 2 +-
server/red-channel-client.c | 20 ++--
server/red-channel-client.h | 14 +++---
server/red-channel.c | 18 +-
server/red-channel.h | 30 ++---
server/red-parse-qxl.c| 2 +-
server/red-worker.c | 2 +-
server/reds-stream.c | 2 +-
server/reds-stream.h | 2 +-
server/reds.c | 12 ++--
server/reds.h | 4 ++--
server/smartcard-channel-client.c | 16
server/smartcard-channel-client.h | 16
server/sound.c| 38 ++---
server/spicevmc.c | 18 +-
server/stream.c | 12 ++--
server/tree.c | 2 +-
server/tree.h | 2 +-
37 files changed, 229 insertions(+), 229 deletions(-)
diff --git a/server/char-device.c b/server/char-device.c
index c40ed65..d325932 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -306,7 +306,7 @@ static void
red_char_device_send_msg_to_clients(RedCharDevice *dev,
}
}
-static int red_char_device_read_from_device(RedCharDevice *dev)
+static bool red_char_device_read_from_device(RedCharDevice *dev)
{
uint64_t max_send_tokens;
int did_read = FALSE;
@@ -720,13 +720,13 @@ static RedCharDeviceClient
*red_char_device_client_new(RedClient *client,
return dev_client;
}
-int red_char_device_client_add(RedCharDevice *dev,
- RedClient *client,
- int do_flow_control,
- uint32_t max_send_queue_size,
- uint32_t num_client_tokens,
- uint32_t num_send_tokens,
- int wait_for_migrate_data)
+bool red_char_device_client_add(RedCharDevice *dev,
+RedClient *client,
+int do_flow_control,
+uint32_t max_send_queue_size,
+
Re: [Spice-devel] bool or gboolean
On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote: > Hi, > question was raised recently on the ML and IRC. > > Some time ago we decided to use gboolean but some of us would like > to discuss again. > > As any style changes there's no right or wrong, mainly personal > opinions but I think consistency is quite important. > > Some consideration (feel free to add/remove/comment) > - gboolean is more used in the code (about 76%) > - TRUE/FALSE are more used (96%) > - bool is C99 convention, defined in stdbool.h > - using gcc the bool type is a bit different from gboolean > (which basically is an int) catching some problems as > warnings (like cast between different function pointers > using bool instead of gboolean/int) > - bool is easier to write (OT: and my vim is more happy too) As noted above gboolean & bool are different types, in fact they are different sizes too (4 bytes vs 1 byte). If you're using glib2, you'll find various places where it wants callbacks which have a gboolean return value, or parameters. You can't pass in a callback which uses bool, as that's not type compatible. So no matter what, you'll always need to use gboolean in some portion of the code, even if you would rather have bool. Thus if consistency is the goal, then gboolean is the winner as you're more likely to be able to kill off all uses of bool, than be able to kill off all uses of gboolean. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
