Created attachment 90019 [details] [review]
patch

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
 **/

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)

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}

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.

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.

I used "val" and not "value" to avoid the confusion with gvalue and genumvalue and functions such as g_enum_get_value.

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.

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(-)

crap. 

Created attachment 105629 [details] [review]
patch

changes:
move back to genum (gtype should be MT safe)
using g_ascii_strto{l,u}l functions.

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(-)

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(-)

Created attachment 119928 [details] [review]
[PATCH] Add G_TYPE_STRING to ENUM,FLAGS

 gobject/gvaluetransform.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

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(-)

Created attachment 119930 [details] [review]
[PATCH] Add flags-transformation tests

 tests/gobject/gvalue-test.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

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(-)

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.

https://bugs.freedesktop.org/show_bug.cgi?id=7763 depends on this bug.

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.

Created attachment 120018 [details] [review]
squashed all

(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.. :(

(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.

(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

Created attachment 120378 [details] [review]
With copyright notice for James Henstridge & libglade contributors

ping!

Tim, Matthias, others, could you please comment. I would really like to this working. 
thank you

(desperate ping for review)

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.

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.

Created attachment 157537 [details] [review]
Add {enum,flags}_from_string functions

Those functions are based on work from James Henstridge & the other
libglade contributors.

Created attachment 157538 [details] [review]
Add GValue STRING to ENUM,FLAGS transform

Created attachment 157539 [details] [review]
tests: modernize gvalue-test and test new functions

What's the usecase for g_flags_get_first_value_from_string?

(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..

(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.

(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.

These kind of API is very useful in parsing command-line arguments.  The GError comes very handy there.

(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)

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.

*** Bug 630304 has been marked as a duplicate of this bug. ***

*** Bug 749360 has been marked as a duplicate of this bug. ***

*** Bug 749360 has been marked as a duplicate of this bug. ***

Created attachment 303358 [details] [review]
Add to_string() functions for Enum and Flags types

(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

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.

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 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").

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.

Before any of this lands, somebody needs to compare this with the code currently used for this purpose in GtkBuilder.

(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.

I'm talking about _gtk_builder_enum_from_string and friends

(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.

Created attachment 329922 [details] [review]
gobject: Add to_string() functions for Enum and Flags types

These are useful for debugging.

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.

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.

(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.

(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" ?

(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.

(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.

Created attachment 343105 [details] [review]
gobject: Add to_string() functions for Enum and Flags types

These are useful for debugging.

Rebased on master; I haven’t changed it to use ‘illegal value’ as per comment #59.

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).

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.

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.

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