From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: master 64e25cd: More robust NS hex colour string parsing Date: Sat, 13 Jun 2020 21:40:13 +0300 Message-ID: <83r1ui3j3m.fsf@gnu.org> References: <20200608120746.30163.87810@vcs0.savannah.gnu.org> <20200608120747.80E8E20A2E@vcs0.savannah.gnu.org> <83r1uk429y.fsf@gnu.org> <3C92A091-F389-4179-B2F0-B3AA5ABD6CCE@acm.org> <83pna43xrl.fsf@gnu.org> <9259B4A6-F3CC-4243-9F08-2882993C9B2C@acm.org> <83a71741mr.fsf@gnu.org> <83y2or2c0o.fsf@gnu.org> <82DE05DD-0F2A-4722-AB75-D9D95F58BAF4@acm.org> <83v9ju3nbl.fsf@gnu.org> <83sgey3m3i.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="46075"; mail-complaints-to="usenet@ciao.gmane.io" Cc: pipcet@gmail.com, emacs-devel@gnu.org To: Mattias =?utf-8?Q?Engdeg=C3=A5rd?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Jun 13 20:41:14 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jkB5e-000Bvh-4j for ged-emacs-devel@m.gmane-mx.org; Sat, 13 Jun 2020 20:41:14 +0200 Original-Received: from localhost ([::1]:36428 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jkB5d-00028a-7K for ged-emacs-devel@m.gmane-mx.org; Sat, 13 Jun 2020 14:41:13 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:35738) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jkB4p-0001FC-Go for emacs-devel@gnu.org; Sat, 13 Jun 2020 14:40:23 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:33353) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jkB4p-0004xr-6o; Sat, 13 Jun 2020 14:40:23 -0400 Original-Received: from [176.228.60.248] (port=1532 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jkB4o-0005TS-FJ; Sat, 13 Jun 2020 14:40:23 -0400 In-Reply-To: (message from Mattias =?utf-8?Q?Engdeg=C3=A5rd?= on Sat, 13 Jun 2020 19:56:02 +0200) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:252197 Archived-At: > From: Mattias Engdegård > 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.