Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 19:56:02 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> > But it isn't rejected by the current code.  Which was my point all
> > along.
> 
> Since "#12345" is malformed it should be rejected, and will be.

That is definitely a change in behavior, isn't it?  For some reason,
you don't regard this point I'm making seriously.  Why not?

> Use a single parser of colour 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 normalisation of the result was sometimes incorrect.
               ^^^^^^^^^^^^^
Our convention is to use the US English spelling.

> +/* Parse fractional hex digits at S ending right before E.
> +   Set *DST to the value normalised to 65535 and return true on success,
> +   false otherwise.  */  ^^^^^^^^^^

Likewise here.

Please also describe in more detail the value put in *DST, I don't
think it's clear enough.

> +/* Parse floating-point number at S ending right before E.
> +   Return the number if in the range [0,1]; otherwise -1.  */
> +static double
> +parse_float_comp (const char *s, const char *e)

The commentary doesn't explain what is the "comp" part of the name
about.

> +/* Parse S as a numeric colour specification and set *R, *G and *B.
                           ^^^^^^
Spelling again.

> +   The result is normalised to a maximum value of 65535 per component.  */
                    ^^^^^^^^^^
And here.

> +DEFUN ("color-values-from-color-spec",
> +       Fcolor_values_from_color_spec, Scolor_values_from_color_spec,
> +       1, 1, 0,
> +       doc: /* Parse STRING as a numeric colour and return (R G B).
                                            ^^^^^^
> +Recognised formats are:
   ^^^^^^^^^^
> + #RGB, where R, G and B are hex strings of equal length, 1-4 digits each
> + rgb:R/G/B, where R, G, and B are hex strings, 1-4 digits each
> + rgbi:R/G/B, where R, G and B are numbers in [0,1].
> +
> +The result is normalised to a maximum value of 65535 per component.
                 ^^^^^^^^^^
> +If STRING is not in one of the above forms, return nil.  */)

Spelling.

I think this doc string is too terse.  I would rephrase the beginning
as follows:

    Convert a color SPEC into a list of standard RGB values.

  Value is a list of the form (R G B), where R, G, and B are the
  integer values, the intensities of the primary colors.
  The argument SPEC should be a string in one of the following formats:

In the "rgbi" description, I think we should mention explicitly that
the components are floating-point numbers.

> +  return (parse_color_spec (SSDATA (string), &r, &g, &b)
> +          ? list3i (r, g, b)
> +          : Qnil);

What happens if the argument is not a string?  What should happen?

Finally, the Lisp primitive needs a NEWS entry and perhaps also a
place in the ELisp manual.

Thanks.