Consolidate #RGB string parsers

Use a single parser of color strings in the #RGB, rgb:R/G/B and
rgbi:R/G/B formats, replacing four existing ones.  Previously,
error-checking was spotty, handling of the rgbi: format not always
present, and normalization of the result was sometimes incorrect.

* src/dispextern.h: New prototype.
* src/xfaces.c (parse_hex_color_comp, parse_float_color_comp)
(parse_color_spec, Finternal-color_values_from_color_spec): New functions.
* test/src/xfaces-tests.el (xfaces-internal-color-values-from-color-spec):
New test.
* lisp/term/tty-colors.el (tty-color-standard-values):
Use internal-color-values-from-color-spec, replacing old parser.
* src/nsterm.m (ns_get_color):
* src/w32fns.c (x_to_w32_color):
* src/xterm.c (x_parse_color): Use parse_color_spec, replacing old
parsers.
(HEX_COLOR_NAME_LENGTH): Remove #define.

7 files changed, 180 insertions, 286 deletions

diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
index 39ca2d36276..73e2431822e 100644
--- a/lisp/term/tty-colors.el
+++ b/lisp/term/tty-colors.el
@@ -923,62 +923,8 @@ The returned value reflects the standard Emacs definition of
 COLOR (see the info node `(emacs) Colors'), regardless of whether
 the terminal can display it, so the return value should be the
 same regardless of what display is being used."
-  (let ((len (length color)))
-    (cond ((and (>= len 4) ;; HTML/CSS/SVG-style "#XXYYZZ" color spec
-                (eq (aref color 0) ?#)
-                (member (aref color 1)
-                        '(?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9
-                             ?a ?b ?c ?d ?e ?f
-                             ?A ?B ?C ?D ?E ?F)))
-           ;; Translate the string "#XXYYZZ" into a list of numbers
-           ;; (XX YY ZZ), scaling each to the {0..65535} range.  This
-           ;; follows the HTML color convention, where both "#fff" and
-           ;; "#ffffff" represent the same color, white.
-           (let* ((ndig (/ (- len 1) 3))
-                  (maxval (1- (ash 1 (* 4 ndig))))
-                  (i1 1)
-                  (i2 (+ i1 ndig))
-                  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-             (list
-              (/ (* (string-to-number
-                     (substring color i1 i2) 16)
-                    65535)
-                 maxval)
-              (/ (* (string-to-number
-                     (substring color i2 i3) 16)
-                    65535)
-                 maxval)
-              (/ (* (string-to-number
-                     (substring color i3 i4) 16)
-                    65535)
-                 maxval))))
-          ((and (>= len 9) ;; X-style rgb:xx/yy/zz color spec
-                (string= (substring color 0 4) "rgb:"))
-           ;; Translate the string "rgb:XX/YY/ZZ" into a list of
-           ;; numbers (XX YY ZZ), scaling each to the {0..65535}
-           ;; range.  "rgb:F/F/F" is white.
-           (let* ((ndig (/ (- len 3) 3))
-                  (maxval (1- (ash 1 (* 4 (- ndig 1)))))
-                  (i1 4)
-                  (i2 (+ i1 ndig))
-                  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-             (list
-              (/ (* (string-to-number
-                     (substring color i1 (- i2 1)) 16)
-                    65535)
-                 maxval)
-              (/ (* (string-to-number
-                     (substring color i2 (- i3 1)) 16)
-                    65535)
-                 maxval)
-              (/ (* (string-to-number
-                     (substring color i3 (1- i4)) 16)
-                    65535)
-                 maxval))))
-          (t
-           (cdr (assoc color color-name-rgb-alist))))))
+  (or (internal-color-values-from-color-spec color)
+      (cdr (assoc color color-name-rgb-alist))))
 
 (defun tty-color-translate (color &optional frame)
   "Given a color COLOR, return the index of the corresponding TTY color.
diff --git a/src/dispextern.h b/src/dispextern.h
index 0b1f3d14aeb..e1d6eddc419 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3514,6 +3514,8 @@ void update_face_from_frame_parameter (struct frame *, Lisp_Object,
                                        Lisp_Object);
 extern bool tty_defined_color (struct frame *, const char *, Emacs_Color *,
                                bool, bool);
+bool parse_color_spec (const char *,
+                       unsigned short *, unsigned short *, unsigned short *);
 
 Lisp_Object tty_color_name (struct frame *, int);
 void clear_face_cache (bool);
diff --git a/src/nsterm.m b/src/nsterm.m
index 3dc7e1db7c9..0e405fc0175 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2341,9 +2341,6 @@ ns_get_color (const char *name, NSColor **col)
    See https://lists.gnu.org/r/emacs-devel/2009-07/msg01203.html.  */
 {
   NSColor *new = nil;
-  static char hex[20];
-  int scaling = 0;
-  float r = -1.0, g, b;
   NSString *nsname = [NSString stringWithUTF8String: name];
 
   NSTRACE ("ns_get_color(%s, **)", name);
@@ -2386,51 +2383,31 @@ ns_get_color (const char *name, NSColor **col)
     }
 
   /* First, check for some sort of numeric specification.  */
-  hex[0] = '\0';
-
-  if (name[0] == '0' || name[0] == '1' || name[0] == '.')  /* RGB decimal */
+  unsigned short r16, g16, b16;
+  if (parse_color_spec (name, &r16, &g16, &b16))
     {
-      NSScanner *scanner = [NSScanner scannerWithString: nsname];
-      [scanner scanFloat: &r];
-      [scanner scanFloat: &g];
-      [scanner scanFloat: &b];
-    }
-  else if (!strncmp(name, "rgb:", 4))  /* A newer X11 format -- rgb:r/g/b */
-    scaling = (snprintf (hex, sizeof hex, "%s", name + 4) - 2) / 3;
-  else if (name[0] == '#')        /* An old X11 format; convert to newer */
-    {
-      int len = 0;
-      while (isxdigit (name[len + 1]))
-        len++;
-      if (name[len + 1] == '\0' && len >= 1 && len <= 12 && len % 3 == 0)
-        {
-          scaling = len / 3;
-          for (int i = 0; i < 3; i++)
-            sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
-                     name + 1 + i * scaling);
-          hex[3 * (scaling + 1) - 1] = '\0';
-        }
+      *col = [NSColor colorForEmacsRed: r16 / 65535.0
+                                 green: g16 / 65535.0
+                                  blue: b16 / 65535.0
+                                 alpha: 1.0];
+      unblock_input ();
+      return 0;
     }
-
-  if (hex[0])
+  else if (name[0] == '0' || name[0] == '1' || name[0] == '.')
     {
-      unsigned int rr, gg, bb;
-      float fscale = (1 << (scaling * 4)) - 1;
-      if (sscanf (hex, "%x/%x/%x", &rr, &gg, &bb))
+      /* RGB decimal */
+      NSScanner *scanner = [NSScanner scannerWithString: nsname];
+      float r, g, b;
+      if (   [scanner scanFloat: &r] && r >= 0 && r <= 1
+          && [scanner scanFloat: &g] && g >= 0 && g <= 1
+          && [scanner scanFloat: &b] && b >= 0 && b <= 1)
         {
-          r = rr / fscale;
-          g = gg / fscale;
-          b = bb / fscale;
+          *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
+          unblock_input ();
+          return 0;
         }
     }
 
-  if (r >= 0.0F)
-    {
-      *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
-      unblock_input ();
-      return 0;
-    }
-
   /* Otherwise, color is expected to be from a list */
   {
     NSEnumerator *lenum, *cenum;
diff --git a/src/w32fns.c b/src/w32fns.c
index e595b0285a7..ab864332e78 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -864,161 +864,14 @@ x_to_w32_color (const char * colorname)
 
   block_input ();
 
-  if (colorname[0] == '#')
+  unsigned short r, g, b;
+  if (parse_color_spec (colorname, &r, &g, &b))
     {
-      /* Could be an old-style RGB Device specification.  */
-      int size = strlen (colorname + 1);
-      char *color = alloca (size + 1);
-
-      strcpy (color, colorname + 1);
-      if (size == 3 || size == 6 || size == 9 || size == 12)
-        {
-          UINT colorval;
-          int i, pos;
-          pos = 0;
-          size /= 3;
-          colorval = 0;
-
-          for (i = 0; i < 3; i++)
-            {
-              char *end;
-              char t;
-              unsigned long value;
-
-              /* The check for 'x' in the following conditional takes into
-                 account the fact that strtol allows a "0x" in front of
-                 our numbers, and we don't.  */
-              if (!isxdigit (color[0]) || color[1] == 'x')
-                break;
-              t = color[size];
-              color[size] = '\0';
-              value = strtoul (color, &end, 16);
-              color[size] = t;
-              if (errno == ERANGE || end - color != size)
-                break;
-              switch (size)
-                {
-                case 1:
-                  value = value * 0x10;
-                  break;
-                case 2:
-                  break;
-                case 3:
-                  value /= 0x10;
-                  break;
-                case 4:
-                  value /= 0x100;
-                  break;
-                }
-              colorval |= (value << pos);
-              pos += 0x8;
-              if (i == 2)
-                {
-                  unblock_input ();
-                  XSETINT (ret, colorval);
-                  return ret;
-                }
-              color = end;
-            }
-        }
-    }
-  else if (strnicmp (colorname, "rgb:", 4) == 0)
-    {
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 4;
-      for (i = 0; i < 3; i++)
-        {
-          char *end;
-          unsigned long value;
-
-          /* The check for 'x' in the following conditional takes into
-             account the fact that strtol allows a "0x" in front of
-             our numbers, and we don't.  */
-          if (!isxdigit (color[0]) || color[1] == 'x')
-            break;
-          value = strtoul (color, &end, 16);
-          if (errno == ERANGE)
-            break;
-          switch (end - color)
-            {
-            case 1:
-              value = value * 0x10 + value;
-              break;
-            case 2:
-              break;
-            case 3:
-              value /= 0x10;
-              break;
-            case 4:
-              value /= 0x100;
-              break;
-            default:
-              value = ULONG_MAX;
-            }
-          if (value == ULONG_MAX)
-            break;
-          colorval |= (value << pos);
-          pos += 0x8;
-          if (i == 2)
-            {
-              if (*end != '\0')
-                break;
-              unblock_input ();
-              XSETINT (ret, colorval);
-              return ret;
-            }
-          if (*end != '/')
-            break;
-          color = end + 1;
-        }
+      unblock_input ();
+      /* Throw away the low 8 bits and return 0xBBGGRR.  */
+      return make_fixnum ((b & 0xff00) << 8 | (g & 0xff00) | r >> 8);
     }
-  else if (strnicmp (colorname, "rgbi:", 5) == 0)
-    {
-      /* This is an RGB Intensity specification.  */
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 5;
-      for (i = 0; i < 3; i++)
-        {
-          char *end;
-          double value;
-          UINT val;
 
-          value = strtod (color, &end);
-          if (errno == ERANGE)
-            break;
-          if (value < 0.0 || value > 1.0)
-            break;
-          val = (UINT)(0x100 * value);
-          /* We used 0x100 instead of 0xFF to give a continuous
-             range between 0.0 and 1.0 inclusive.  The next statement
-             fixes the 1.0 case.  */
-          if (val == 0x100)
-            val = 0xFF;
-          colorval |= (val << pos);
-          pos += 0x8;
-          if (i == 2)
-            {
-              if (*end != '\0')
-                break;
-              unblock_input ();
-              XSETINT (ret, colorval);
-              return ret;
-            }
-          if (*end != '/')
-            break;
-          color = end + 1;
-        }
-    }
   /* I am not going to attempt to handle any of the CIE color schemes
      or TekHVC, since I don't know the algorithms for conversion to
      RGB.  */
diff --git a/src/xfaces.c b/src/xfaces.c
index cf155288bd1..308509a0267 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -220,6 +220,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "sysstdio.h"
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <math.h>
 
 #include "lisp.h"
 #include "character.h"
@@ -819,6 +820,120 @@ load_pixmap (struct frame *f, Lisp_Object name)
                             Color Handling
  ***********************************************************************/
 
+/* Parse hex color component at S ending right before E.
+   Set *DST to the value normalized so that the maximum for the
+   number of digits given becomes 65535, and return true on success,
+   false otherwise.  */
+static bool
+parse_hex_color_comp (const char *s, const char *e, unsigned short *dst)
+{
+  int n = e - s;
+  if (n <= 0 || n > 4)
+    return false;
+  int val = 0;
+  for (; s < e; s++)
+    {
+      int digit;
+      if (*s >= '0' && *s <= '9')
+        digit = *s - '0';
+      else if (*s >= 'A' && *s <= 'F')
+        digit = *s - 'A' + 10;
+      else if (*s >= 'a' && *s <= 'f')
+        digit = *s - 'a' + 10;
+      else
+        return false;
+      val = (val << 4) | digit;
+    }
+  int maxval = (1 << (n * 4)) - 1;
+  *dst = (unsigned)val * 65535 / maxval;
+  return true;
+}
+
+/* Parse floating-point color component at S ending right before E.
+   Return the number if in the range [0,1]; otherwise -1.  */
+static double
+parse_float_color_comp (const char *s, const char *e)
+{
+  char *end;
+  double x = strtod (s, &end);
+  return (end == e && x >= 0 && x <= 1) ? x : -1;
+}
+
+/* Parse S as a numeric color specification and set *R, *G and *B.
+   Return true on success, false on failure.
+   Recognized formats:
+
+    "#RGB", with R, G and B hex strings of equal length, 1-4 digits each
+    "rgb:R/G/B", with R, G and B hex strings, 1-4 digits each
+    "rgbi:R/G/B", with R, G and B numbers in [0,1]
+
+   The result is normalized to a maximum value of 65535 per component.  */
+bool
+parse_color_spec (const char *s,
+                  unsigned short *r, unsigned short *g, unsigned short *b)
+{
+  int len = strlen (s);
+  if (s[0] == '#')
+    {
+      if ((len - 1) % 3 == 0)
+        {
+          int n = (len - 1) / 3;
+          return (   parse_hex_color_comp (s + 1 + 0 * n, s + 1 + 1 * n, r)
+                  && parse_hex_color_comp (s + 1 + 1 * n, s + 1 + 2 * n, g)
+                  && parse_hex_color_comp (s + 1 + 2 * n, s + 1 + 3 * n, b));
+        }
+    }
+  else if (strncmp (s, "rgb:", 4) == 0)
+    {
+      char *sep1, *sep2;
+      return ((sep1 = strchr (s + 4, '/')) != NULL
+              && (sep2 = strchr (sep1 + 1, '/')) != NULL
+              && parse_hex_color_comp (s + 4, sep1, r)
+              && parse_hex_color_comp (sep1 + 1, sep2, g)
+              && parse_hex_color_comp (sep2 + 1, s + len, b));
+    }
+  else if (strncmp (s, "rgbi:", 5) == 0)
+    {
+      char *sep1, *sep2;
+      double red, green, blue;
+      if ((sep1 = strchr (s + 5, '/')) != NULL
+          && (sep2 = strchr (sep1 + 1, '/')) != NULL
+          && (red = parse_float_color_comp (s + 5, sep1)) >= 0
+          && (green = parse_float_color_comp (sep1 + 1, sep2)) >= 0
+          && (blue = parse_float_color_comp (sep2 + 1, s + len)) >= 0)
+        {
+          *r = lrint (red * 65535);
+          *g = lrint (green * 65535);
+          *b = lrint (blue * 65535);
+          return true;
+        }
+    }
+  return false;
+}
+
+DEFUN ("internal-color-values-from-color-spec",
+       Finternal_color_values_from_color_spec,
+       Sinternal_color_values_from_color_spec,
+       1, 1, 0,
+       doc: /* Parse STRING as a numeric color and return (RED GREEN BLUE).
+Recognised formats for STRING are:
+
+ #RGB, where R, G and B are hex numbers of equal length, 1-4 digits each
+ rgb:R/G/B, where R, G, and B are hex numbers, 1-4 digits each
+ rgbi:R/G/B, where R, G and B are floating-point numbers in [0,1]
+
+The result is normalized to a maximum value of 65535 per component,
+forming a list of three integers in [0,65535].
+If STRING is not in one of the above forms, return nil.  */)
+  (Lisp_Object string)
+{
+  CHECK_STRING (string);
+  unsigned short r, g, b;
+  return (parse_color_spec (SSDATA (string), &r, &g, &b)
+          ? list3i (r, g, b)
+          : Qnil);
+}
+
 /* Parse RGB_LIST, and fill in the RGB fields of COLOR.
    RGB_LIST should contain (at least) 3 lisp integers.
    Return true iff RGB_LIST is OK.  */
@@ -7018,4 +7133,5 @@ clear the face cache, see `clear-face-cache'.  */);
   defsubr (&Sinternal_face_x_get_resource);
   defsubr (&Sx_family_fonts);
 #endif
+  defsubr (&Sinternal_color_values_from_color_spec);
 }
diff --git a/src/xterm.c b/src/xterm.c
index 7989cecec7f..6340700cb89 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -2376,8 +2376,6 @@ x_query_frame_background_color (struct frame *f, XColor *bgcolor)
   x_query_colors (f, bgcolor, 1);
 }
 
-#define HEX_COLOR_NAME_LENGTH 32
-
 /* On frame F, translate the color name to RGB values.  Use cached
    information, if possible.
 
@@ -2389,44 +2387,23 @@ x_query_frame_background_color (struct frame *f, XColor *bgcolor)
 Status x_parse_color (struct frame *f, const char *color_name,
                       XColor *color)
 {
+  /* Don't pass #RGB strings directly to XParseColor, because that
+     follows the X convention of zero-extending each channel
+     value: #f00 means #f00000.  We want the convention of scaling
+     channel values, so #f00 means #ff0000, just as it does for
+     HTML, SVG, and CSS.  */
+  unsigned short r, g, b;
+  if (parse_color_spec (color_name, &r, &g, &b))
+    {
+      color->red = r;
+      color->green = g;
+      color->blue = b;
+      return 1;
+    }
+
   Display *dpy = FRAME_X_DISPLAY (f);
   Colormap cmap = FRAME_X_COLORMAP (f);
   struct color_name_cache_entry *cache_entry;
-
-  if (color_name[0] == '#')
-    {
-      /* Don't pass #RGB strings directly to XParseColor, because that
-         follows the X convention of zero-extending each channel
-         value: #f00 means #f00000.  We want the convention of scaling
-         channel values, so #f00 means #ff0000, just as it does for
-         HTML, SVG, and CSS.
-
-         So we translate #f00 to rgb:f/0/0, which X handles
-         differently. */
-      char rgb_color_name[HEX_COLOR_NAME_LENGTH];
-      int len = strlen (color_name);
-      int digits_per_channel;
-      if (len == 4)
-        digits_per_channel = 1;
-      else if (len == 7)
-        digits_per_channel = 2;
-      else if (len == 10)
-        digits_per_channel = 3;
-      else if (len == 13)
-        digits_per_channel = 4;
-      else
-        return 0;
-
-      snprintf (rgb_color_name, sizeof rgb_color_name, "rgb:%.*s/%.*s/%.*s",
-                digits_per_channel, color_name + 1,
-                digits_per_channel, color_name + digits_per_channel + 1,
-                digits_per_channel, color_name + 2 * digits_per_channel + 1);
-
-      /* The rgb form is parsed directly by XParseColor without
-         talking to the X server.  No need for caching.  */
-      return XParseColor (dpy, cmap, rgb_color_name, color);
-    }
-
   for (cache_entry = FRAME_DISPLAY_INFO (f)->color_names; cache_entry;
        cache_entry = cache_entry->next)
     {
diff --git a/test/src/xfaces-tests.el b/test/src/xfaces-tests.el
index 5ed16c9e51d..34cda07e5b4 100644
--- a/test/src/xfaces-tests.el
+++ b/test/src/xfaces-tests.el
@@ -24,4 +24,27 @@
   (should (equal (color-distance "#222222" "#ffffff")
                  (color-distance "#ffffff" "#222222"))))
 
+(ert-deftest xfaces-internal-color-values-from-color-spec ()
+  (should (equal (internal-color-values-from-color-spec "#f05")
+                 '(#xffff #x0000 #x5555)))
+  (should (equal (internal-color-values-from-color-spec "#1fb0C5")
+                 '(#x1f1f #xb0b0 #xc5c5)))
+  (should (equal (internal-color-values-from-color-spec "#1f8b0AC5e")
+                 '(#x1f81 #xb0aa #xc5eb)))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e2")
+                 '(#x1f83 #xb0ad #xc5e2)))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e2g") nil))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e20") nil))
+  (should (equal (internal-color-values-from-color-spec "#12345") nil))
+  (should (equal (internal-color-values-from-color-spec "rgb:f/23/28a")
+                 '(#xffff #x2323 #x28a2)))
+  (should (equal (internal-color-values-from-color-spec "rgb:1234/5678/09ab")
+                 '(#x1234 #x5678 #x09ab)))
+  (should (equal (internal-color-values-from-color-spec "rgb:0//0") nil))
+  (should (equal (internal-color-values-from-color-spec "rgbi:0/0.5/0.1")
+                 '(0 32768 6554)))
+  (should (equal (internal-color-values-from-color-spec "rgbi:1e-3/1.0e-2/1e0")
+                 '(66 655 65535)))
+  (should (equal (internal-color-values-from-color-spec "rgbi:0/0.5/10") nil)))
+
 (provide 'xfaces-tests)