Created attachment 369729 [details] [review]
gst_structure_to_string: display actual value of pointers

We used to always display "NULL" which was pretty confusing when
debugging.

Review of attachment 369729 [details] [review]:

::: gst/gststructure.c
@@ +1833,2 @@
     } else {
+      if (!G_TYPE_CHECK_VALUE_TYPE (&field->value, G_TYPE_STRING))

I'm not sure why we special case strings here as they are supposed to be serialized by gst_value_serialize().

I was wondering about that part as well (G_TYPE_STRING).

I would like a unit test to go with this that makes sure that we don't deserialize these pointer values.

Created attachment 370584 [details] [review]
gst_structure_to_string: display actual value of pointers

We used to always display "NULL" which was pretty confusing when
debugging.

Created attachment 372630 [details] [review]
gst_structure_to_string: display actual value of pointers

We used to always display "NULL" which was pretty confusing when
debugging.

Rebased.
Is this good to go?

Comment on attachment 372630 [details] [review]
gst_structure_to_string: display actual value of pointers

I think this is good to go, but I'm not sure about the unit test:

>--- a/tests/check/gst/gststructure.c
>+++ b/tests/check/gst/gststructure.c
>@@ -274,6 +274,18 @@ GST_START_TEST (test_to_from_string)
> 
>   gst_structure_free (st1);
>   gst_structure_free (st2);
>+
>+  /* pointers are serialized but not deserialized */
>+  st1 = gst_structure_new ("test", "ptr", G_TYPE_POINTER, 0xdeadbeef, NULL);
>+  str = gst_structure_to_string (st1);
>+  fail_unless (strcmp (str, "test, ptr=(gpointer)0xdeadbeef;") == 0,
>+      "Failed to serialize to right string: %s", str);

I'm not sure if we can assume that g_string_append_printf() will yield the exact same string on every system. Unless we know it will always end up in an internal glib printf implementation and never a system printf. e.g. it could be 0xDEADBEEF on some systems, or maybe an extra prefix or so. I don't know if the C standard specifies exactly how this must be printed - did you check?

Good point, it's indeed platform specific: https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gprintf.c#L315

I relaxed the check in the test.

Created attachment 372689 [details] [review]
gst_structure_to_string: display actual value of pointers

We used to always display "NULL" which was pretty confusing when
debugging.

Comment on attachment 372689 [details] [review]
gst_structure_to_string: display actual value of pointers

Thanks!

Attachment 372689 [details] pushed as 6dba0d9 - gst_structure_to_string: display actual value of pointers