[units] Be more strict in the grammar we are parsing

Current parsing of units has a number of shortcomings:
  * a number followed by trailing space (without any unit specified) was
    not recognized,
  * "5 emeralds" was parsed as 5em,
  * the way we parse the digits after the separator makes us lose
    precision for no good reason (5.0 is parsed as 5.00010014...f which
    makes g_assert_cmpfloat() fail)

Let's define a stricter grammar we can recognize and try to do so. The
description is in EBNF form, removing the optional <> which is a pain
when having to write DocBook, and using '' for the terminal symbols.

Last step, add more ClutterUnits unit test to get a better coverage of
the grammar we want to parse.

Reviewed-by: Emmanuele Bassi <ebassi@linux.intel.com>

diff --git a/clutter/clutter-units.c b/clutter/clutter-units.c
index 788bab9..069bed0 100644 (file)
--- a/clutter/clutter-units.c
+++ b/clutter/clutter-units.c
@@ -362,10 +362,14 @@ clutter_units_to_pixels (ClutterUnits *units)
  * A #ClutterUnits expressed in string should match:
  *
  * |[
- *   number: [0-9]
- *   unit_value: <number>+
- *   unit_name: px|pt|mm|em
- *   units: <unit_value> <unit_name>
+ *   units: wsp* unit-value wsp* unit-name? wsp*
+ *   unit-value: number
+ *   unit-name: 'px' | 'pt' | 'mm' | 'em'
+ *   number: digit+
+ *           | digit* sep digit+
+ *   sep: '.' | ','
+ *   digit: '0' | '1' | '2' | '3' | '4' | '5' | '6' | '7' | '8' | '9'
+ *   wsp: (#0x20 | #0x9 | #0xA | #0xB | #0xC | #0xD)+
  * ]|
  *
  * For instance, these are valid strings:
@@ -375,6 +379,7 @@ clutter_units_to_pixels (ClutterUnits *units)
  *   5.1 em
  *   24 pt
  *   12.6 mm
+ *   .3 cm
  * ]|
  *
  * While these are not:
@@ -401,51 +406,66 @@ clutter_units_from_string (ClutterUnits *units,
   g_return_val_if_fail (units != NULL, FALSE);
   g_return_val_if_fail (str != NULL, FALSE);
 
-  /* Ensure that the first character is a digit */
+  /* strip leading space */
   while (g_ascii_isspace (*str))
     str++;
 
   if (*str == '\0')
     return FALSE;
 
-  if (!g_ascii_isdigit (*str))
-    return FALSE;
-
   /* integer part */
   value = (gfloat) strtoul (str, (char **) &str, 10);
 
   if (*str == '.' || *str == ',')
     {
-      glong frac = 100000;
+      gfloat divisor = 0.1;
+
+      /* 5.cm is not a valid number */
+      if (!g_ascii_isdigit (*++str))
+        return FALSE;
 
-      while (g_ascii_isdigit (*++str))
+      while (g_ascii_isdigit (*str))
         {
-          frac += (*str - '0') * frac;
-          frac /= 10;
+          value += (*str - '0') * divisor;
+          divisor *= 0.1;
+          str++;
         }
-
-      value += (1.0f / (gfloat) frac);
     }
 
+  while (g_ascii_isspace (*str))
+    str++;
+
   /* assume pixels by default, if no unit is specified */
   if (*str == '\0')
     unit_type = CLUTTER_UNIT_PIXEL;
-  else
+  else if (strncmp (str, "em", 2) == 0)
     {
-      while (g_ascii_isspace (*str))
-        str++;
-
-      if (strncmp (str, "em", 2) == 0)
-        unit_type = CLUTTER_UNIT_EM;
-      else if (strncmp (str, "mm", 2) == 0)
-        unit_type = CLUTTER_UNIT_MM;
-      else if (strncmp (str, "pt", 2) == 0)
-        unit_type = CLUTTER_UNIT_POINT;
-      else if (strncmp (str, "px", 2) == 0)
-        unit_type = CLUTTER_UNIT_PIXEL;
-      else
-        return FALSE;
+      unit_type = CLUTTER_UNIT_EM;
+      str += 2;
     }
+  else if (strncmp (str, "mm", 2) == 0)
+    {
+      unit_type = CLUTTER_UNIT_MM;
+      str += 2;
+    }
+  else if (strncmp (str, "pt", 2) == 0)
+    {
+      unit_type = CLUTTER_UNIT_POINT;
+      str += 2;
+    }
+  else if (strncmp (str, "px", 2) == 0)
+    {
+      unit_type = CLUTTER_UNIT_PIXEL;
+      str += 2;
+    }
+  else
+        return FALSE;
+
+  /* ensure the unit is only followed by white space */
+  while (g_ascii_isspace (*str))
+    str++;
+  if (*str != '\0')
+    return FALSE;
 
   units->unit_type = unit_type;
   units->value = value;

diff --git a/tests/conform/test-clutter-units.c b/tests/conform/test-clutter-units.c
index 2a8f8ba..3506eaf 100644 (file)
--- a/tests/conform/test-clutter-units.c
+++ b/tests/conform/test-clutter-units.c
@@ -31,14 +31,36 @@ test_units_string (TestConformSimpleFixture *fixture,
   g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_PIXEL);
   g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 10);
 
+  g_assert (clutter_units_from_string (&units, "10  ") == TRUE);
+  g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_PIXEL);
+  g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 10);
+
   g_assert (clutter_units_from_string (&units, "5 em") == TRUE);
   g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_EM);
   g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 5);
 
+  g_assert (clutter_units_from_string (&units, "5 emeralds") == FALSE);
+
   g_assert (clutter_units_from_string (&units, "  16   mm") == TRUE);
   g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_MM);
   g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 16);
 
+  g_assert (clutter_units_from_string (&units, "  24   pt   ") == TRUE);
+  g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_POINT);
+  g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 24);
+
+  g_assert (clutter_units_from_string (&units, "  32   em   garbage") == FALSE);
+
+  g_assert (clutter_units_from_string (&units, "5.1mm") == TRUE);
+  g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_MM);
+  g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 5.1f);
+
+  g_assert (clutter_units_from_string (&units, "5,mm") == FALSE);
+
+  g_assert (clutter_units_from_string (&units, ".5pt") == TRUE);
+  g_assert (clutter_units_get_unit_type (&units) == CLUTTER_UNIT_POINT);
+  g_assert_cmpfloat (clutter_units_get_unit_value (&units), ==, 0.5f);
+
   g_assert (clutter_units_from_string (&units, "1 pony") == FALSE);
 
   clutter_units_from_pt (&units, 24.0);