[glib: 5/19] gbinding: Canonicalise source and target properties

[Emmanuele Bassi <ebassi src gnome org>]

commit ae27f503426087590785e8c40d0c91b574993593
Author: Philip Withnall <withnall endlessm com>
Date:   Tue Nov 12 19:29:19 2019 +0000

    gbinding: Canonicalise source and target properties
    
    Rather than interning a property name string which isn’t canonicalised,
    canonicalise it first, and enforce stricter validation on inputs.
    
    The previous code was not incorrect (since the property machinery would
    have canonicalised the property names itself, internally), but would
    have resulted in non-canonical property names getting into the GQuark
    table unnecessarily. With the new code, the interned property names from
    property installation time should be consistently reused.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Helps: #358

 gobject/gbinding.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++-----
 gobject/tests/binding.c | 32 ++++++++++++++++++
 2 files changed, 113 insertions(+), 8 deletions(-)
---
diff --git a/gobject/gbinding.c b/gobject/gbinding.c
index 16942c221..bed3fce10 100644
--- a/gobject/gbinding.c
+++ b/gobject/gbinding.c
@@ -423,6 +423,53 @@ g_binding_finalize (GObject *gobject)
   G_OBJECT_CLASS (g_binding_parent_class)->finalize (gobject);
 }
 
+/* @key must have already been validated with is_valid()
+ * Modifies @key in place. */
+static void
+canonicalize_key (gchar *key)
+{
+  gchar *p;
+
+  for (p = key; *p != 0; p++)
+    {
+      gchar c = *p;
+
+      if (c == '_')
+        *p = '-';
+    }
+}
+
+/* @key must have already been validated with is_valid() */
+static gboolean
+is_canonical (const gchar *key)
+{
+  return (strchr (key, '_') == NULL);
+}
+
+static gboolean
+is_valid_property_name (const gchar *key)
+{
+  const gchar *p;
+
+  /* First character must be a letter. */
+  if ((key[0] < 'A' || key[0] > 'Z') &&
+      (key[0] < 'a' || key[0] > 'z'))
+    return FALSE;
+
+  for (p = key; *p != 0; p++)
+    {
+      const gchar c = *p;
+
+      if (c != '-' && c != '_' &&
+          (c < '0' || c > '9') &&
+          (c < 'A' || c > 'Z') &&
+          (c < 'a' || c > 'z'))
+        return FALSE;
+    }
+
+  return TRUE;
+}
+
 static void
 g_binding_set_property (GObject      *gobject,
                         guint         prop_id,
@@ -437,17 +484,35 @@ g_binding_set_property (GObject      *gobject,
       binding->source = g_value_get_object (value);
       break;
 
-    case PROP_SOURCE_PROPERTY:
-      binding->source_property = g_intern_string (g_value_get_string (value));
-      break;
-
     case PROP_TARGET:
       binding->target = g_value_get_object (value);
       break;
 
+    case PROP_SOURCE_PROPERTY:
     case PROP_TARGET_PROPERTY:
-      binding->target_property = g_intern_string (g_value_get_string (value));
-      break;
+      {
+        gchar *name_copy = NULL;
+        const gchar *name = g_value_get_string (value);
+        const gchar **dest;
+
+        /* Ensure the name we intern is canonical. */
+        if (!is_canonical (name))
+          {
+            name_copy = g_value_dup_string (value);
+            canonicalize_key (name_copy);
+            name = name_copy;
+          }
+
+        if (prop_id == PROP_SOURCE_PROPERTY)
+          dest = &binding->source_property;
+        else
+          dest = &binding->target_property;
+
+        *dest = g_intern_string (name);
+
+        g_free (name_copy);
+        break;
+      }
 
     case PROP_FLAGS:
       binding->flags = g_value_get_flags (value);
@@ -606,7 +671,10 @@ g_binding_class_init (GBindingClass *klass)
    * GBinding:source-property:
    *
    * The name of the property of #GBinding:source that should be used
-   * as the source of the binding
+   * as the source of the binding.
+   *
+   * This should be in [canonical form][canonical-parameter-names] to get the
+   * best performance.
    *
    * Since: 2.26
    */
@@ -622,7 +690,10 @@ g_binding_class_init (GBindingClass *klass)
    * GBinding:target-property:
    *
    * The name of the property of #GBinding:target that should be used
-   * as the target of the binding
+   * as the target of the binding.
+   *
+   * This should be in [canonical form][canonical-parameter-names] to get the
+   * best performance.
    *
    * Since: 2.26
    */
@@ -835,8 +906,10 @@ g_object_bind_property_full (gpointer               source,
 
   g_return_val_if_fail (G_IS_OBJECT (source), NULL);
   g_return_val_if_fail (source_property != NULL, NULL);
+  g_return_val_if_fail (is_valid_property_name (source_property), NULL);
   g_return_val_if_fail (G_IS_OBJECT (target), NULL);
   g_return_val_if_fail (target_property != NULL, NULL);
+  g_return_val_if_fail (is_valid_property_name (target_property), NULL);
 
   if (source == target && g_strcmp0 (source_property, target_property) == 0)
     {
diff --git a/gobject/tests/binding.c b/gobject/tests/binding.c
index 019fb280e..0226a29f9 100644
--- a/gobject/tests/binding.c
+++ b/gobject/tests/binding.c
@@ -313,6 +313,37 @@ binding_default (void)
   g_assert (binding == NULL);
 }
 
+static void
+binding_canonicalisation (void)
+{
+  BindingSource *source = g_object_new (binding_source_get_type (), NULL);
+  BindingTarget *target = g_object_new (binding_target_get_type (), NULL);
+  GBinding *binding;
+
+  g_test_summary ("Test that bindings set up with non-canonical property names work");
+
+  binding = g_object_bind_property (source, "double_value",
+                                    target, "double_value",
+                                    G_BINDING_DEFAULT);
+
+  g_object_add_weak_pointer (G_OBJECT (binding), (gpointer *) &binding);
+  g_assert_true ((BindingSource *) g_binding_get_source (binding) == source);
+  g_assert_true ((BindingTarget *) g_binding_get_target (binding) == target);
+  g_assert_cmpstr (g_binding_get_source_property (binding), ==, "double-value");
+  g_assert_cmpstr (g_binding_get_target_property (binding), ==, "double-value");
+  g_assert_cmpint (g_binding_get_flags (binding), ==, G_BINDING_DEFAULT);
+
+  g_object_set (source, "double-value", 24.0, NULL);
+  g_assert_cmpfloat (target->double_value, ==, source->double_value);
+
+  g_object_set (target, "double-value", 69.0, NULL);
+  g_assert_cmpfloat (source->double_value, !=, target->double_value);
+
+  g_object_unref (target);
+  g_object_unref (source);
+  g_assert_null (binding);
+}
+
 static void
 binding_bidirectional (void)
 {
@@ -734,6 +765,7 @@ main (int argc, char *argv[])
   g_test_bug_base ("https://gitlab.gnome.org/GNOME/glib/issues/";);
 
   g_test_add_func ("/binding/default", binding_default);
+  g_test_add_func ("/binding/canonicalisation", binding_canonicalisation);
   g_test_add_func ("/binding/bidirectional", binding_bidirectional);
   g_test_add_func ("/binding/transform", binding_transform);
   g_test_add_func ("/binding/transform-default", binding_transform_default);