Re: [PATCH 1/1] all: add C99's "bool" define and encourage its use

2015-11-18 Thread Thomas Haller
Merged the patch to master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=37824def11c2c7947103ae668ec487bc90c10c35


Thomas

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list


Re: [PATCH 1/1] all: add C99's "bool" define and encourage its use

2015-11-12 Thread Thomas Haller
On Wed, 2015-11-11 at 11:00 -0600, Dan Williams wrote:
> On Wed, 2015-11-11 at 13:33 +0100, Thomas Haller wrote:
> > ---
> >  Let's use bool.
> > 
> >  Comments?
> 
> Since glib uses gboolean in quite a few places, would we have
> problems
> with casting it to gboolean if needed?  Casting would be pretty ugly
> if
> we had to do it a bunch.

No problem, no cast needed. Assigning gboolean to _Bool and vice versa
would always yield either 0 or 1 in the result. Contrary to assigning a
gboolean to a gboolean, which possibly yields a result in range
[G_MININT, G_MAXINT].

Also, setting a GObject property of type boolean will just work:

    _Bool x = TRUE;
    g_object_set (obj, NM_PROP_BOOL, x, NULL);

because the variadic argument type will be promoted to int.

Ok, the getter does require extra work:
    gboolean tmp;
    _Bool x;
    g_object_get (obj, NM_PROP_BOOL, &tmp, NULL);
    x = tmp;




> Also, I don't think we'd save any space because the compiler is going
> to
> pad any field 1-byte field (like bool) out to 4 or 8 bytes anyway,
> unless the structure is packed.

Obviously, you have to consider struct alignment for having any saving.
In the worst cast, it uses as much memory as gboolean.




> Lastly, using bitfields in structs is not generally recommended since
> apparently (at least) gcc creates awful code for accesses.  At least
> that's the argument in kernel-land for not doing bitfields, and you
> never see them there.

I don't want to use now bitfields everywhere. But with gboolean you
*cannot*. With _Bool, whether a field is a bitfield or not has almost
no difference in how you can use it.

I'd like to see _Bool as an accepted datatype in NetworkManager. I
think it's preferable to gboolean in most situations, except for the
fact that gboolean is wildly used and by introducing _Bool you must be
aware of yet another type. But then, _Bool is hardly a complicated type
and even C99 standard.


Thomas






> >  Thomas
> > 
> >  include/nm-default.h | 46
> > ++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/include/nm-default.h b/include/nm-default.h
> > index d5c9d10..dc02763 100644
> > --- a/include/nm-default.h
> > +++ b/include/nm-default.h
> > @@ -70,4 +70,50 @@
> >  
> >  /*
> > /
> >  
> > +/**
> > + * The boolean type _Bool is C99 but we mostly stick to C89.
> > However, _Bool is too
> > + * convinient to miss and is effectively available in gcc and
> > clang. So, just use it.
> > + *
> > + * Usually, one would include "stdbool.h" to get the bool define
> > which aliases
> > + * _Bool. We provide this define here, because we want to make use
> > of it anywhere.
> > + * (also, stdbool.h is again C99).
> > + *
> > + * Using _Bool has advantages over gboolean:
> > + *
> > + * - commonly, _Bool is one byte large, instead of gboolean's 4
> > bytes (because gboolean
> > + *   is a typedef for gint). Especially when having boolean fields
> > in a struct, we can
> > + *   thereby easily save some space.
> > + *
> > + * - _Bool type is guaranteed that two true expressions compare
> > equal. E.g. the follwing
> > + *   will not work:
> > + *gboolean v1 = 1;
> > + *gboolean v2 = 2;
> > + *g_assert_cmpint (v1, ==, v2); // will fail
> > + *   For that, we often to use !! to coerce gboolean values to 0
> > or 1:
> > + *g_assert_cmpint (!!v2, ==, TRUE);
> > + *   With _Bool this will be properly done by the compiler.
> > + *
> > + * - For structs, we might want to safe even more space and use
> > bitfields:
> > + *   struct s1 {
> > + *   gboolean v1:1;
> > + *   };
> > + *   But the problem here is that gboolean is signed, so that
> > + *   v1 will be either 0 or -1 (not 1, TRUE). Thus, the following
> > + *   doesn't work:
> > + *  struct s1 s = { .v1 = TRUE, };
> > + *  g_assert_cmpint (s1.v1, ==, TRUE);
> > + *   It will however work just fine with bool/_Bool.
> > + *
> > + * Also, add the defines for true/false. Those are nicely
> > highlighted by the editor
> > + * as special types, contrary to glib's TRUE/FALSE.
> > + */
> > +
> > +#ifndef bool
> > +#define bool _Bool
> > +#define true1
> > +#define false   0
> > +#endif
> > +
> > +/*
> > /
> > +
> >  #endif /* __NM_DEFAULT_H__ */
> 
> 

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list


Re: [PATCH 1/1] all: add C99's "bool" define and encourage its use

2015-11-11 Thread Dan Williams
On Wed, 2015-11-11 at 13:33 +0100, Thomas Haller wrote:
> ---
>  Let's use bool.
> 
>  Comments?

Since glib uses gboolean in quite a few places, would we have problems
with casting it to gboolean if needed?  Casting would be pretty ugly if
we had to do it a bunch.

Also, I don't think we'd save any space because the compiler is going to
pad any field 1-byte field (like bool) out to 4 or 8 bytes anyway,
unless the structure is packed.

Lastly, using bitfields in structs is not generally recommended since
apparently (at least) gcc creates awful code for accesses.  At least
that's the argument in kernel-land for not doing bitfields, and you
never see them there.

Dan

>  Thomas
> 
>  include/nm-default.h | 46 ++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/include/nm-default.h b/include/nm-default.h
> index d5c9d10..dc02763 100644
> --- a/include/nm-default.h
> +++ b/include/nm-default.h
> @@ -70,4 +70,50 @@
>  
>  
> /*/
>  
> +/**
> + * The boolean type _Bool is C99 but we mostly stick to C89. However, _Bool 
> is too
> + * convinient to miss and is effectively available in gcc and clang. So, 
> just use it.
> + *
> + * Usually, one would include "stdbool.h" to get the bool define which 
> aliases
> + * _Bool. We provide this define here, because we want to make use of it 
> anywhere.
> + * (also, stdbool.h is again C99).
> + *
> + * Using _Bool has advantages over gboolean:
> + *
> + * - commonly, _Bool is one byte large, instead of gboolean's 4 bytes 
> (because gboolean
> + *   is a typedef for gint). Especially when having boolean fields in a 
> struct, we can
> + *   thereby easily save some space.
> + *
> + * - _Bool type is guaranteed that two true expressions compare equal. E.g. 
> the follwing
> + *   will not work:
> + *gboolean v1 = 1;
> + *gboolean v2 = 2;
> + *g_assert_cmpint (v1, ==, v2); // will fail
> + *   For that, we often to use !! to coerce gboolean values to 0 or 1:
> + *g_assert_cmpint (!!v2, ==, TRUE);
> + *   With _Bool this will be properly done by the compiler.
> + *
> + * - For structs, we might want to safe even more space and use bitfields:
> + *   struct s1 {
> + *   gboolean v1:1;
> + *   };
> + *   But the problem here is that gboolean is signed, so that
> + *   v1 will be either 0 or -1 (not 1, TRUE). Thus, the following
> + *   doesn't work:
> + *  struct s1 s = { .v1 = TRUE, };
> + *  g_assert_cmpint (s1.v1, ==, TRUE);
> + *   It will however work just fine with bool/_Bool.
> + *
> + * Also, add the defines for true/false. Those are nicely highlighted by the 
> editor
> + * as special types, contrary to glib's TRUE/FALSE.
> + */
> +
> +#ifndef bool
> +#define bool _Bool
> +#define true1
> +#define false   0
> +#endif
> +
> +/*/
> +
>  #endif /* __NM_DEFAULT_H__ */


___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list