After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 447907 - [PATCH] enum/flags from string + type transform + tests
[PATCH] enum/flags from string + type transform + tests
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
2.12.x
Other Linux
: Normal enhancement
: ---
Assigned To: Tim Janik
gtkdev
: 630304 749360 (view as bug list)
Depends on:
Blocks: 327198 767765
 
 
Reported: 2007-06-15 14:59 UTC by Johan (not receiving bugmail) Dahlin
Modified: 2017-03-30 09:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (2.02 KB, patch)
2007-06-15 15:01 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
g_type_{enum,flags}_val_from_string (5.56 KB, patch)
2008-02-19 23:43 UTC, Marc-Andre Lureau
none Details | Review
[1/1] Add g_mount_mount_flags_get_type (844 bytes, patch)
2008-02-20 10:33 UTC, Marc-Andre Lureau
none Details | Review
patch (5.77 KB, patch)
2008-02-20 10:37 UTC, Marc-Andre Lureau
none Details | Review
[PATCH] Use GTest for gvalue-test, and fix formatting (12.46 KB, patch)
2008-10-04 18:12 UTC, Marc-Andre Lureau
rejected Details | Review
[PATCH] Add {enum,flags}_from_string functions (6.69 KB, patch)
2008-10-04 18:12 UTC, Marc-Andre Lureau
rejected Details | Review
[PATCH] Add G_TYPE_STRING to ENUM,FLAGS (1.59 KB, patch)
2008-10-04 18:12 UTC, Marc-Andre Lureau
rejected Details | Review
[PATCH] Add a test of enum transformation to string (2.62 KB, patch)
2008-10-04 18:12 UTC, Marc-Andre Lureau
rejected Details | Review
[PATCH] Add flags-transformation tests (1.95 KB, patch)
2008-10-04 18:12 UTC, Marc-Andre Lureau
rejected Details | Review
[PATCH] Fix a memleak (better for test self-documentation) (401 bytes, patch)
2008-10-04 18:13 UTC, Marc-Andre Lureau
rejected Details | Review
squashed all (22.18 KB, patch)
2008-10-06 12:37 UTC, Marc-Andre Lureau
none Details | Review
With copyright notice for James Henstridge & libglade contributors (22.57 KB, patch)
2008-10-11 13:25 UTC, Marc-Andre Lureau
reviewed Details | Review
Add {enum,flags}_from_string functions (7.27 KB, patch)
2010-03-30 22:30 UTC, Marc-Andre Lureau
none Details | Review
Add GValue STRING to ENUM,FLAGS transform (1.96 KB, patch)
2010-03-30 22:31 UTC, Marc-Andre Lureau
needs-work Details | Review
tests: modernize gvalue-test and test new functions (14.01 KB, patch)
2010-03-30 22:35 UTC, Marc-Andre Lureau
needs-work Details | Review
Add to_string() functions for Enum and Flags types (6.88 KB, patch)
2015-05-14 10:33 UTC, Garrett Regier
none Details | Review
Add to_string() functions for Enum and Flags types v2 (9.59 KB, patch)
2015-05-14 15:00 UTC, Garrett Regier
none Details | Review
Add to_string() functions for Enum and Flags types v3 (8.59 KB, patch)
2015-05-19 09:02 UTC, Garrett Regier
none Details | Review
gobject: Add to_string() functions for Enum and Flags types (8.54 KB, patch)
2016-06-16 23:03 UTC, Philip Withnall
none Details | Review
gobject: Add to_string() functions for Enum and Flags types (8.54 KB, patch)
2017-01-07 23:39 UTC, Philip Withnall
committed Details | Review

Description Johan (not receiving bugmail) Dahlin 2007-06-15 14:59:11 UTC
It'd be nice to have a g_enum_get_from_string which which would make it easier to parse a enum from a string based on the nick, name and number.
Comment 1 Johan (not receiving bugmail) Dahlin 2007-06-15 15:01:36 UTC
Created attachment 90019 [details] [review]
patch
Comment 2 Behdad Esfahbod 2007-06-15 15:21:36 UTC
For reference, I have this utility function in pango:

/**
 * pango_parse_enum:
 * @type: enum type to parse, eg. %PANGO_TYPE_ELLIPSIZE_MODE.
 * @str: string to parse.  May be %NULL.
 * @value: integer to store the result in, or %NULL.
 * @warn: if %TRUE, issue a g_warning() on bad input.
 * @possible_values: place to store list of possible values on failure, or %NULL.
 *
 * Parses an enum type and stored the result in @value.
 *
 * If @str does not match the nick name of any of the possible values for the
 * enum, %FALSE is returned, a warning is issued if @warn is %TRUE, and a
 * string representing the list of possible values is stored in
 * @possible_values.  The list is slash-separated, eg.
 * "none/start/middle/end".  If failed and @possible_values is not %NULL,
 * returned string should be freed using g_free().
 *
 * Return value: %TRUE if @str was successfully parsed.
 *
 * Since: 1.16
 **/
Comment 3 Marc-Andre Lureau 2008-02-19 12:01:42 UTC
Hi

+1 form me. There is similar code in gstreamer and gtkbuilder. It would really make sense.

I would consider _gtk_builder_{enum,flags}_from_string, which has a decent parsing (including numbers).

(/me would like to use it from Vala)
Comment 4 Marc-Andre Lureau 2008-02-19 12:43:45 UTC
In the other way, to_string would be also a nice function. I have seen it in some projects, like gstreamer, glade, and others.

Take for example: 
glade_property_class_make_string_from_{flags,enum}
Comment 5 Marc-Andre Lureau 2008-02-19 20:44:07 UTC
When we have g_enum_get_from_string, it would be really nice to have a value_transform_string_enum.

(In reply to comment #4)
value_transform_enum_string should use this to_string() for example.

Comment 6 Marc-Andre Lureau 2008-02-19 23:43:20 UTC
Created attachment 105616 [details] [review]
g_type_{enum,flags}_val_from_string

here is the enum/flag from string, based on gtkbuilder.c (thanks johan;) and with the comment of pango ;)

I removed the GError code, since gobject does not use it at all and it was not really useful anyway.
I also don't think the idea to return a list of possible values is really interesteding in a gtype generic api.
I move the stuff to gtype, because everybody is doing the same type_class_ref trick to get the type and it does not make so much sens to leave that to each program to me, and a g_type_{enum,flags} set of function looks like it would fit in gtype.c to me.
Comment 7 Marc-Andre Lureau 2008-02-19 23:51:20 UTC
I used "val" and not "value" to avoid the confusion with gvalue and genumvalue and functions such as g_enum_get_value.
Comment 8 Marc-Andre Lureau 2008-02-20 08:39:19 UTC
We have to be careful, since developpers have been using this nick and name as a free-form field name in different places:

 gst_rtsp_lower_trans_get_type (void)
{
  static GType rtsp_lower_trans_type = 0;
  static const GFlagsValue rtsp_lower_trans[] = {
    {RTSP_LOWER_TRANS_UDP, "UDP Unicast Mode", "udp-unicast"},

...

Maybe the documentation should give guideline and the enum registration should check the nick/name string validity, issuing a warning when incorrect.
Comment 9 Marc-Andre Lureau 2008-02-20 10:33:30 UTC
Created attachment 105627 [details] [review]
[1/1] Add g_mount_mount_flags_get_type



git-svn-id: svn+ssh://svn.gnome.org/svn/glib/trunk@6501 5bbd4a9e-d125-0410-bf1d-f987e7eefc80
---
 gio/ChangeLog   |    4 ++++
 gio/gio.symbols |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)
Comment 10 Marc-Andre Lureau 2008-02-20 10:34:52 UTC
crap. 
Comment 11 Marc-Andre Lureau 2008-02-20 10:37:10 UTC
Created attachment 105629 [details] [review]
patch

changes:
move back to genum (gtype should be MT safe)
using g_ascii_strto{l,u}l functions.
Comment 12 Marc-Andre Lureau 2008-10-04 18:12:31 UTC
Created attachment 119926 [details] [review]
[PATCH] Use GTest for gvalue-test, and fix formatting

 tests/gobject/Makefile.am   |    6 +-
 tests/gobject/gvalue-test.c |  245 ++++++++++++++++++++++---------------------
 2 files changed, 131 insertions(+), 120 deletions(-)
Comment 13 Marc-Andre Lureau 2008-10-04 18:12:43 UTC
Created attachment 119927 [details] [review]
[PATCH] Add {enum,flags}_from_string functions

 docs/reference/gobject/gobject-sections.txt |    3 +
 gobject/genums.c                            |  163 +++++++++++++++++++++++++++
 gobject/genums.h                            |    7 +-
 gobject/gobject.symbols                     |    5 +-
 4 files changed, 176 insertions(+), 2 deletions(-)
Comment 14 Marc-Andre Lureau 2008-10-04 18:12:47 UTC
Created attachment 119928 [details] [review]
[PATCH] Add G_TYPE_STRING to ENUM,FLAGS

 gobject/gvaluetransform.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)
Comment 15 Marc-Andre Lureau 2008-10-04 18:12:53 UTC
Created attachment 119929 [details] [review]
[PATCH] Add a test of enum transformation to string

 tests/gobject/gvalue-test.c |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)
Comment 16 Marc-Andre Lureau 2008-10-04 18:12:57 UTC
Created attachment 119930 [details] [review]
[PATCH] Add flags-transformation tests

 tests/gobject/gvalue-test.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)
Comment 17 Marc-Andre Lureau 2008-10-04 18:13:01 UTC
Created attachment 119931 [details] [review]
[PATCH] Fix a memleak (better for test self-documentation)

 tests/gobject/gvalue-test.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Comment 18 Marc-Andre Lureau 2008-10-04 18:32:39 UTC
The return documentation for g_flags_get_value_from_string is wrong, it returns the value.

The function could have more constness, but then it's not compatible with the function used.
Comment 19 Marc-Andre Lureau 2008-10-04 21:48:29 UTC
https://bugs.freedesktop.org/show_bug.cgi?id=7763 depends on this bug.
Comment 20 Tim Janik 2008-10-06 11:08:56 UTC
Please don't attach lots of individual git patches to bugzilla. Git makes this easy, yes, but it sucks nevertheless. All too soon, we soon loose track of the individual patches, reviews, comments and updates to them. Please create and attach one squashed patch that you're proposing for glib inclusion. If something really warrants a seperate patch, it most probably warranbts a separate bug report as well.
Comment 21 Marc-Andre Lureau 2008-10-06 12:37:42 UTC
Created attachment 120018 [details] [review]
squashed all
Comment 22 Marc-Andre Lureau 2008-10-06 12:38:45 UTC
(In reply to comment #20)
> Please don't attach lots of individual git patches to bugzilla. 

Yes.. I wish we would have a mailing list. I really feel we loose something by squashing them all.. :(
Comment 23 Johan (not receiving bugmail) Dahlin 2008-10-06 12:47:44 UTC
(In reply to comment #21)
> Created an attachment (id=120018) [edit]
> squashed all
> 

I think that you should mention that James Henstridge & the other libglade contributors has the copyright, as most of the code is taken directly from libglade.
Comment 24 Marc-Andre Lureau 2008-10-06 13:44:05 UTC
(In reply to comment #23)
> I think that you should mention that James Henstridge & the other libglade
> contributors has the copyright, as most of the code is taken directly from
> libglade.
> 
sure
Comment 25 Marc-Andre Lureau 2008-10-11 13:25:40 UTC
Created attachment 120378 [details] [review]
With copyright notice for James Henstridge & libglade contributors

ping!
Comment 26 Marc-Andre Lureau 2008-10-17 19:53:42 UTC
Tim, Matthias, others, could you please comment. I would really like to this working. 
thank you
Comment 27 Marc-Andre Lureau 2008-10-31 23:55:07 UTC
(desperate ping for review)
Comment 28 Emmanuele Bassi (:ebassi) 2010-01-01 17:39:45 UTC
the same code now lives in at least three other projects:

  • Clutter (for the UI definition files)
  • JSON-GLib (serialization and deserialization of properties)
  • LibUnique (serialization and deserialization of commands)

I'm going to do a short review of the patch in the hope to get the ball rolling for inclusion in GLib 2.24, because frankly the situation is getting a little bit out of hand.
Comment 29 Emmanuele Bassi (:ebassi) 2010-01-01 17:50:03 UTC
Review of attachment 120378 [details] [review]:

the patch looks good to me.

I feel that the GValue-from-string transformation should live in its own patch, though I understand the need to squash it in the pre-Splinter days when we didn't have a propert review tool. :-)

::: gobject/genums.c
@@ +687,3 @@
+ * g_flags_get_value_from_string:
+ * @flags_class: the flags class to translate to
+ * @string: string to parse containing a numeric or `|' separated list

since this function uses the UTF-8 API to scan the string we should probably add a notice in the documentation, like:

  @string: a UTF-8 string, containing...

also, "containing a numeric" what? did you mean "containing a numeric value"?

@@ +715,3 @@
+      && endptr == (string + strlen (string))) /* parsed a number */
+    {
+      if (value & ~flags_class->mask) {

code style police: the curly brace should be indented here

@@ +722,3 @@
+  else
+    {
+

code style police: unnecessary whitespace

@@ +726,3 @@
+      for (value = i = j = 0; ; i++)
+	{
+

code style police: unnecessary whitespace

::: tests/gobject/gvalue-test.c
@@ +147,3 @@
 static void
+test_gtype_value (void         *unused,
+                  gconstpointer test_data)

starting from this point it's all whitespace changes; can you please split it out into its own commit? no need to review it - you could even commit it right now.
Comment 30 Marc-Andre Lureau 2010-03-30 22:30:39 UTC
Created attachment 157537 [details] [review]
Add {enum,flags}_from_string functions

Those functions are based on work from James Henstridge & the other
libglade contributors.
Comment 31 Marc-Andre Lureau 2010-03-30 22:31:57 UTC
Created attachment 157538 [details] [review]
Add GValue STRING to ENUM,FLAGS transform
Comment 32 Marc-Andre Lureau 2010-03-30 22:35:53 UTC
Created attachment 157539 [details] [review]
tests: modernize gvalue-test and test new functions
Comment 33 Behdad Esfahbod 2010-03-31 14:08:09 UTC
What's the usecase for g_flags_get_first_value_from_string?
Comment 34 Marc-Andre Lureau 2010-03-31 14:57:10 UTC
(In reply to comment #33)
> What's the usecase for g_flags_get_first_value_from_string?

I think that was for consistency API reason:

const GEnumValue *   enum_from_string()
const GFlagsValue *  first_flag_from_string()
guint                flags_from_string ()

perhaps unnecessary then..
Comment 35 Behdad Esfahbod 2010-03-31 15:09:17 UTC
(In reply to comment #34)
> (In reply to comment #33)
> > What's the usecase for g_flags_get_first_value_from_string?
> 
> I think that was for consistency API reason:
> 
> const GEnumValue *   enum_from_string()
> const GFlagsValue *  first_flag_from_string()
> guint                flags_from_string ()
> 
> perhaps unnecessary then..

The non-string API allows for enumerating all the flags set in an int.  So it's useful.  The string version, if we want it, should return the first flag parsed in the string and a pointer to the next position in the string.  But I'm not sure that's really useful.

Also, I suggest returning a GError with proper message when parsing fails.  Perhaps listing all the available nicks if there's just a few.
Comment 36 Marc-Andre Lureau 2010-03-31 15:41:47 UTC
(In reply to comment #35)
> The non-string API allows for enumerating all the flags set in an int.  So it's
> useful.  The string version, if we want it, should return the first flag parsed
> in the string and a pointer to the next position in the string.  But I'm not
> sure that's really useful.

That sound too complicated, and it's probably stepping into a zone of special case. At this point, we should make a choice of simplicty vs everything-you-need.

> Also, I suggest returning a GError with proper message when parsing fails. 
> Perhaps listing all the available nicks if there's just a few.

Nothing in gobject/* uses GError. Also, it's not a general case that you want to report that kind of error to the user: Couldn't read value from string: "FOO". I suppose those values should not be edited manually anyway, and thus should not have error.

Except if having different enum/flags API: a future version of code adding an enum literal, and both versions using the same string... so versionising is up to the application in that case, imho.
Comment 37 Behdad Esfahbod 2010-03-31 15:47:56 UTC
These kind of API is very useful in parsing command-line arguments.  The GError comes very handy there.
Comment 38 Marc-Andre Lureau 2010-03-31 15:55:35 UTC
(In reply to comment #37)
> These kind of API is very useful in parsing command-line arguments.  The GError
> comes very handy there.

ok, assuming it is, I suppose the function returning a NULL pointer don't need it. So we are probably talking about g_flags_get_value_from_string () only.

I don't mind adding it back if there is a consensus (I think it was there when I adapted the code from glade, or pango ;) ?) It just looks odd to me that only this function would have GError and nothing else in gobject reports error (there could be many places where error would also be interesting, but it was left to the developer to figure out)
Comment 39 Behdad Esfahbod 2010-03-31 16:01:24 UTC
These are clearly utility functions.  So I wouldn't care if they are the only GError-using functions in gobject.

Plus, the NULL-returning functions can use returning an error message just as well.

That's just my opinion though.
Comment 40 Matthias Clasen 2010-09-22 01:17:00 UTC
*** Bug 630304 has been marked as a duplicate of this bug. ***
Comment 41 Emmanuele Bassi (:ebassi) 2015-05-14 09:59:21 UTC
*** Bug 749360 has been marked as a duplicate of this bug. ***
Comment 42 Garrett Regier 2015-05-14 10:31:09 UTC
*** Bug 749360 has been marked as a duplicate of this bug. ***
Comment 43 Garrett Regier 2015-05-14 10:33:31 UTC
Created attachment 303358 [details] [review]
Add to_string() functions for Enum and Flags types
Comment 44 Garrett Regier 2015-05-14 10:34:57 UTC
(In reply to Garrett Regier from comment #43)
> Created attachment 303358 [details] [review] [review]
> Add to_string() functions for Enum and Flags types

This is useful for debugging. It would be nice, for usability, to also have a function which took a GType and not just GEnumsClass/GFlagsClass.

In fact for GEnumsClass it is so easy to get the to_string() value that it might make sense to not have a function for it and only have one taking a enum GType.

Builder currently has a flags_to_string() function which is used for debugging: https://git.gnome.org/browse/gnome-builder/tree/contrib/egg/egg-binding-set.c#n80
Comment 45 Marc-Andre Lureau 2015-05-14 13:35:34 UTC
Review of attachment 303358 [details] [review]:

those enum/flags to string conversion already exist in value_transform_enum_string() and value_transform_flags_string(). No need for new functions imho, or you should either refactor or wrap the code differently.
Comment 46 Garrett Regier 2015-05-14 15:00:50 UTC
Created attachment 303375 [details] [review]
Add to_string() functions for Enum and Flags types v2

(In reply to Marc-Andre Lureau from comment #45)
> Review of attachment 303358 [details] [review] [review]:
> 
> those enum/flags to string conversion already exist in
> value_transform_enum_string() and value_transform_flags_string(). No need
> for new functions imho, or you should either refactor or wrap the code
> differently.

These functions do not fallback and/or include the enum/flags value and instead return NULL. The transform functions now use the appropriate to_string() function.
Comment 47 Dan Winship 2015-05-16 13:38:58 UTC
Comment on attachment 303375 [details] [review]
Add to_string() functions for Enum and Flags types v2

>+gchar *
>+g_enum_to_string (GType g_enum_type,
>+                  gint  value)

It would be useful to have this return a const char *, so that you can do things like:

  void
  foo_do_whatever (Foo *foo, int n, FooArg arg)
  {
    g_debug ("foo_do_whatever(%x, %d, %s)", foo, n,
             g_enum_to_string (FOO_TYPE_ARG, arg));
    ...

This could be done by interning the string if you don't want to assume the GEnumClass will stay reffed. (Interning would work in the flags case too where you actually have to construct the string.)

>+  if (enum_value != NULL)
>+    result = g_strdup (enum_value->value_name);

For many purposes (like the debug logging example above), it would be better to guarantee that this always returns a string. Possibly g_strdup_printf("%d", value)

>+ * For example: "G_BINDING_DEFAULT" or
>+                "G_BINDING_BIDIRECTIONAL | "G_BINDING_SYNC_CREATE".

there's an extra quote in the second line

>+g_flags_get_value_string (GFlagsClass *flags_class,
>+g_flags_to_string (GType g_flags_type,

I don't see why we need both of these.

>+      if (!first)
>+        g_string_append (str, " | ");

You don't need @first; you can just check if str->len is > 0.

As in the enums case, it would be useful to include any "extra" bits in the result as well (" | 0x%x").
Comment 48 Garrett Regier 2015-05-19 09:02:10 UTC
Created attachment 303576 [details] [review]
Add to_string() functions for Enum and Flags types v3

(In reply to Dan Winship from comment #47)
> Comment on attachment 303375 [details] [review] [review]
> Add to_string() functions for Enum and Flags types v2
> 
> >+gchar *
> >+g_enum_to_string (GType g_enum_type,
> >+                  gint  value)
> 
> It would be useful to have this return a const char *, so that you can do
> things like:
> 
>   void
>   foo_do_whatever (Foo *foo, int n, FooArg arg)
>   {
>     g_debug ("foo_do_whatever(%x, %d, %s)", foo, n,
>              g_enum_to_string (FOO_TYPE_ARG, arg));
>     ...
> 
> This could be done by interning the string if you don't want to assume the
> GEnumClass will stay reffed. (Interning would work in the flags case too
> where you actually have to construct the string.)
> 

This could be done, however it looks like none of the other debugging to_string() functions return an interned string.

> >+  if (enum_value != NULL)
> >+    result = g_strdup (enum_value->value_name);
> 
> For many purposes (like the debug logging example above), it would be better
> to guarantee that this always returns a string. Possibly
> g_strdup_printf("%d", value)
> 

Done.

> >+ * For example: "G_BINDING_DEFAULT" or
> >+                "G_BINDING_BIDIRECTIONAL | "G_BINDING_SYNC_CREATE".
> 
> there's an extra quote in the second line
> 

Fixed.

> >+g_flags_get_value_string (GFlagsClass *flags_class,
> >+g_flags_to_string (GType g_flags_type,
> 
> I don't see why we need both of these.
> 

I did it this way because most of the API revolves around GFlagsClass, so to keep that same theme I added both functions. This most calls can avoid having to ref/unref the flags class. We could, of course, simplify things to just the to_string() function. Really, the get_value_string() function was only added for completeness.

> >+      if (!first)
> >+        g_string_append (str, " | ");
> 
> You don't need @first; you can just check if str->len is > 0.
> 

Fixed.

> As in the enums case, it would be useful to include any "extra" bits in the
> result as well (" | 0x%x").

Done.
Comment 49 Matthias Clasen 2015-05-19 12:37:29 UTC
Before any of this lands, somebody needs to compare this with the code currently used for this purpose in GtkBuilder.
Comment 50 Garrett Regier 2015-05-19 12:47:29 UTC
(In reply to Matthias Clasen from comment #49)
> Before any of this lands, somebody needs to compare this with the code
> currently used for this purpose in GtkBuilder.

I'm assuming you are talking about the from_string() patches as a grep over gtk+ doesn't show any usage of get_first_value(). My patch is about to_string() not from_string() and as such shouldn't duplicate any code in GtkBuilder.
Comment 51 Matthias Clasen 2015-05-19 14:41:50 UTC
I'm talking about _gtk_builder_enum_from_string and friends
Comment 52 Garrett Regier 2015-05-19 14:46:08 UTC
(In reply to Matthias Clasen from comment #51)
> I'm talking about _gtk_builder_enum_from_string and friends

Again, this is about to_string() not from_string() and as such that function is not comparable to those in the patch.
Comment 53 Philip Withnall 2016-06-16 23:03:27 UTC
Created attachment 329922 [details] [review]
gobject: Add to_string() functions for Enum and Flags types

These are useful for debugging.
Comment 54 Philip Withnall 2016-06-16 23:04:51 UTC
I've been using g_flags_to_string() in bug #767765, and updated the patch for it so it applies on master and has the correct 'Since' lines. Updated version attached.

I think it would be rather good if this went in, since it seems fairly uncontroversial. The from_string() stuff is a bit more controversial, but they don't have to go in together.
Comment 55 Matthias Clasen 2016-06-21 01:51:06 UTC
Review of attachment 329922 [details] [review]:

I'm not convinced we really want to add string formatting apis like this. It is so random

::: gobject/genums.c
@@ +574,3 @@
+ * Pretty-prints @value in the form of the enum's name.
+ *
+ * For example: "G_FILE_TYPE_REGULAR".

This gets into personal preference, but I much prefer nicks over formal names when printing out enums and flags...

@@ +601,3 @@
+    result = g_strdup_printf ("%d", value);
+  else
+    result = g_strdup (enum_value->value_name);

That seems a rather gratitious strdup for the fringe case that somebody wants to format a value thats not in the enum.
Comment 56 Philip Withnall 2016-06-22 02:11:05 UTC
(In reply to Matthias Clasen from comment #55)
> Review of attachment 329922 [details] [review] [review]:
> 
> I'm not convinced we really want to add string formatting apis like this. It
> is so random

They are very useful for debug logging. The alternative is a lot of hand-written my_enum_to_string() methods, which are a pain to write.

> ::: gobject/genums.c
> @@ +574,3 @@
> + * Pretty-prints @value in the form of the enum's name.
> + *
> + * For example: "G_FILE_TYPE_REGULAR".
> 
> This gets into personal preference, but I much prefer nicks over formal
> names when printing out enums and flags...

I would tend to agree. I think the original intention of this patch was to allow the formatted version to be round-tripped back to an int using a from_string() API, for which you need to use the full name. That’s not relevant for the debug use case.

> @@ +601,3 @@
> +    result = g_strdup_printf ("%d", value);
> +  else
> +    result = g_strdup (enum_value->value_name);
> 
> That seems a rather gratitious strdup for the fringe case that somebody
> wants to format a value thats not in the enum.

What would you suggest otherwise? I guess my suggestion would be to intern the g_strdup_printf("%d") string (as Dan suggested a few comments ago), and to make corresponding changes to g_flags_to_string(). That would make the APIs nicer to use (no memory management, woo!), but at the cost of inflating the interned string hash, potentially by a lot if someone uses g_enum_to_string() in a loop for some reason.
Comment 57 Matthias Clasen 2016-06-29 14:35:17 UTC
(In reply to Philip Withnall from comment #56)

> What would you suggest otherwise? I guess my suggestion would be to intern
> the g_strdup_printf("%d") string (as Dan suggested a few comments ago), and
> to make corresponding changes to g_flags_to_string(). That would make the
> APIs nicer to use (no memory management, woo!), but at the cost of inflating
> the interned string hash, potentially by a lot if someone uses
> g_enum_to_string() in a loop for some reason.

Just return "illegal value" ?
Comment 58 Philip Withnall 2016-06-29 14:39:33 UTC
(In reply to Matthias Clasen from comment #57)
> (In reply to Philip Withnall from comment #56)
> 
> > What would you suggest otherwise? I guess my suggestion would be to intern
> > the g_strdup_printf("%d") string (as Dan suggested a few comments ago), and
> > to make corresponding changes to g_flags_to_string(). That would make the
> > APIs nicer to use (no memory management, woo!), but at the cost of inflating
> > the interned string hash, potentially by a lot if someone uses
> > g_enum_to_string() in a loop for some reason.
> 
> Just return "illegal value" ?

Works for me (for both flags and enums). However, it means that the round-trip use case with g_[flags|enum]_from_string() is no longer possible. I’m fine with that.

New patch coming soon.
Comment 59 Philip Withnall 2016-06-29 14:45:46 UTC
(In reply to Philip Withnall from comment #58)
> (In reply to Matthias Clasen from comment #57)
> > (In reply to Philip Withnall from comment #56)
> > 
> > > What would you suggest otherwise? I guess my suggestion would be to intern
> > > the g_strdup_printf("%d") string (as Dan suggested a few comments ago), and
> > > to make corresponding changes to g_flags_to_string(). That would make the
> > > APIs nicer to use (no memory management, woo!), but at the cost of inflating
> > > the interned string hash, potentially by a lot if someone uses
> > > g_enum_to_string() in a loop for some reason.
> > 
> > Just return "illegal value" ?
> 
> Works for me (for both flags and enums). However, it means that the
> round-trip use case with g_[flags|enum]_from_string() is no longer possible.
> I’m fine with that.
> 
> New patch coming soon.

Hrm, actually the g_strdup() is necessary anyway (even if we didn’t have the g_strdup_printf() for the unknown value) since we unref the GTypeClass afterwards, and hence potentially invalidate all the type information including the value names and nicks.
Comment 60 Philip Withnall 2017-01-07 23:39:38 UTC
Created attachment 343105 [details] [review]
gobject: Add to_string() functions for Enum and Flags types

These are useful for debugging.
Comment 61 Philip Withnall 2017-01-07 23:40:19 UTC
Rebased on master; I haven’t changed it to use ‘illegal value’ as per comment #59.
Comment 62 Philip Withnall 2017-03-23 14:45:43 UTC
Review of attachment 157539 [details] [review]:

::: tests/gobject/Makefile.am
@@ +57,3 @@
+
+TEST_PROGS          += gvalue-test
+gvalue_test_SOURCES  = gvalue-test.c

This seems wrong: test_programs is the list of installed-tests to run, which is where we’re aiming for tests to be listed (once they’re converted to TAP, which this patch does).

::: tests/gobject/gvalue-test.c
@@ +55,3 @@
+  g_value_init (&xform, G_TYPE_UCHAR);
+  g_value_transform (&orig, &xform);
+  g_assert_cmpint (g_value_get_uchar (&xform), ==, 1);

g_assert_cmpuint() (and in various other places in the file).
Comment 63 Philip Withnall 2017-03-23 14:50:21 UTC
Review of attachment 157538 [details] [review]:

::: gobject/gvaluetransform.c
@@ +245,3 @@
+{
+  GEnumClass *class = g_type_class_ref (G_VALUE_TYPE (dest_value));
+  const GEnumValue* enum_value;

Nitpick: space before the `*` rather than after it.

@@ +255,2 @@
+}
+static void

Nitpick: Blank line between the functions.

@@ +258,3 @@
+                              GValue       *dest_value)
+{
+  GFlagsClass *class = g_type_class_ref (G_VALUE_TYPE (dest_value));

Don’t use `class` as a variable name, as I suspect a C++ compiler will explode on it.
Comment 64 Allison Karlitskaya (desrt) 2017-03-23 16:18:38 UTC
Review of attachment 343105 [details] [review]:

::: gobject/genums.c
@@ +575,3 @@
+ *
+ * For example: "G_FILE_TYPE_REGULAR".
+ *

Note: this function is intended to be used for debugging purposes.  The format of the output may change in the future.

@@ +624,3 @@
+ */
+gchar *
+

You make this function private, and we have a deal.

@@ +668,3 @@
+ * For example: "G_BINDING_DEFAULT" or
+ *              "G_BINDING_BIDIRECTIONAL | G_BINDING_SYNC_CREATE".
+ *              "G_BINDING_BIDIRECTIONAL | G_BINDING_SYNC_CREATE".

Note: this function is intended to be used for debugging purposes.  The format of the output may change in the future.
Comment 65 Philip Withnall 2017-03-30 09:12:01 UTC
Pushed with those changes, thanks for the review.

I’m going to close this bug without merging the *_from_string() patches, since interest seems to mostly be in the *_to_string() case (since it’s immediately useful for debugging). If anybody has a strong desire to add *_from_string() and round-tripping for parsing flags/enums, please open a new bug report. Though personally I think that kind of code belongs in applications, not GLib, since it’s pretty niche. Or it should be implemented using some kind of GType serialisation which works for all GTypes.

Attachment 343105 [details] pushed as 6c95cd2 - gobject: Add to_string() functions for Enum and Flags types