From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: =?UTF-8?Q?Linus_Bj=C3=B6rnstam?= Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Add srfi-171 to guile Date: Sun, 05 Jan 2020 14:34:35 +0100 Message-ID: References: <87mub2ywcc.fsf@pobox.com> Mime-Version: 1.0 Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="131193"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Cyrus-JMAP/3.1.7-694-gd5bab98-fmstable-20191218v1 Cc: guile-devel To: "Andy Wingo" Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Sun Jan 05 14:35:24 2020 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1io63v-000Xsf-5E for guile-devel@m.gmane.org; Sun, 05 Jan 2020 14:35:24 +0100 Original-Received: from localhost ([::1]:42340 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1io63t-0003hG-VG for guile-devel@m.gmane.org; Sun, 05 Jan 2020 08:35:21 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:34582) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1io63a-0003GV-3Y for guile-devel@gnu.org; Sun, 05 Jan 2020 08:35:04 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1io63Y-0007p3-1k for guile-devel@gnu.org; Sun, 05 Jan 2020 08:35:01 -0500 Original-Received: from out2-smtp.messagingengine.com ([66.111.4.26]:48323) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1io63X-0007o3-LK for guile-devel@gnu.org; Sun, 05 Jan 2020 08:34:59 -0500 Original-Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 15356210D8; Sun, 5 Jan 2020 08:34:58 -0500 (EST) Original-Received: from imap1 ([10.202.2.51]) by compute3.internal (MEProxy); Sun, 05 Jan 2020 08:34:58 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=veryfast.biz; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type:content-transfer-encoding; s=fm1; bh=yE8pv EnJsAx3as19e7zIe7+Eqs6jEjgybhHtJ8+/9gw=; b=bT8B+NtHw/DrJSv4Fdbt9 XmDxfDBmeux+B/5NOI5lk+oXhRHa6W6vuvQuaVt3TluQdqJFONlTiaEz4YNZwA/H LZUpftrecIYLcjFqiVbbhhNdhHMngBGPVMeWsQXnIq84e6wUGrsqgpLYhmpjPKpN YjvOfQ9T/CTfBXUhXgbZ3KerEQNT9YvQra8Hsh4dcke4HlNcxt8nuAPWz+jW9eKQ xzZ/4polsZsloD/e9pE7Vm9QejM82xj754xSLclsiUrse1VkqRDuEArJHhwALFiK 36TE0Jf4hH0IcpGY6RkVGqWr2a2ixTahAkGy0FGYgRp8Cu5PEEN5aOiAKPIsUASI Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=yE8pvEnJsAx3as19e7zIe7+Eqs6jEjgybhHtJ8+/9 gw=; b=RDVgTIjEexpMnE/3QPmQH71tSLvtNz8tPOZt38uadCpd1XAl/upQUMi8x +jPGU/rDtytLeb1+rLwGiGq7rdluW+YZA3aPqwFsvOqPQ/9b5N91NgA9soI4gUHq UOvwXLh0HFjWKWlhGO+iRIOE+eBlnk8vDwGx0rUR6NXEYtpJMLVQlDbi9Vn/5+1x YiQplX7FHiSg9Nzc1WPwIsZ1x+y+YVDWqehZyatQYw3AcnZDsssey4FeOyHp4Rwm zQbsSbJQQknEVfmx9HrMd0kD0i8v9mEAS7Hq30hdN/1XOBaB79lowOit8mS+jSyc 91yiBkFvby23YvyybUwXucSCfxh0g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrvdegkedgheefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgfgsehtqhertderreejnecuhfhrohhmpefnihhn uhhspgeujhpnrhhnshhtrghmuceolhhinhhushdrsghjohhrnhhsthgrmhesvhgvrhihfh grshhtrdgsihiiqeenucffohhmrghinheptghhrghrrggtthgvrhhsrddqqddquggvvhdp shgthhgvmhgvrhhsrdhorhhgpdhgnhhurdhorhhgnecurfgrrhgrmhepmhgrihhlfhhroh hmpehlihhnuhhsrdgsjhhorhhnshhtrghmsehvvghrhihfrghsthdrsghiiienucevlhhu shhtvghrufhiiigvpedt X-ME-Proxy: Original-Received: by mailuser.nyi.internal (Postfix, from userid 501) id D1E65C200A4; Sun, 5 Jan 2020 08:34:57 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface In-Reply-To: <87mub2ywcc.fsf@pobox.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.111.4.26 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: "guile-devel" Xref: news.gmane.org gmane.lisp.guile.devel:20221 Archived-At: Thanks for taking time to review it! I will make a new patch with all your proposed changes. Don't mind the correctness comments, btw. They are a rest from the first= successful implementation when I had a less clear mental model of how t= ransducers worked. I know how to verify and test that they work as they = should.=20 Regarding the copyright block I was a bit too trigger happy. I completel= y forgot! I have loads at work right now (2 weeks of modern music cd recording. Fu= n times!), but so will make sure to try to get everything done before th= e end of January. Best regards Linus Bj=C3=B6rnstam On Sun, 5 Jan 2020, at 12:30, Andy Wingo wrote: > Hi :) >=20 > Since this is a final SRFI I think there's no problem getting it in. > Some formatting notes follow; since it's your first Guile patch I'm a > bit verbose :) Probably this will miss 3.0.0 but make 3.0.1, FWIW. >=20 > On Sun 22 Dec 2019 15:55, Linus Bj=C3=B6rnstam writes: >=20 > > From 7e8d3b22ba5f814c40dbb5ab616a318c0cdc2f3e Mon Sep 17 00:00:00 20= 01 > > From: =3D?UTF-8?q?Linus=3D20Bj=3DC3=3DB6rnstam?=3D > > Date: Sun, 22 Dec 2019 15:38:34 +0100 > > Subject: [PATCH 1/2] Added srfi-171 to guile under the module name (= srfi > > srfi-171). > > > > For more info, read the SRFI document: https://srfi.schemers.org/srf= i-171/srfi-171.html >=20 > Needs a note per-file; see other commit log messages. Also please wra= p > to 72 characters. >=20 > > --- /dev/null > > +++ b/module/srfi/srfi-171.scm > > @@ -0,0 +1,498 @@ > > +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;= ;;;;;;;;;;;;;; > > +;; Copyright 2019 Linus Bj.rnstam > > +;; >=20 > I think you've assigned copyright so this can have the standard Guile > copyright block, right? >=20 > > +;; This module name is guile-specific. The correct module name is o= f course > > +;; (srfi 171) >=20 > I don't think it's right to say there is a "correct" name. R6RS, R7RS= , > and Guile have different naming conventions for SRFI modules and that'= s > OK. >=20 > The style in Guile is generally that block comments like this should b= e > complete sentences, starting with capital letters and including > terminating punctuation. Generally we do two spaces after periods, > also. >=20 > > +(define-module (srfi srfi-171) > > + #:declarative? #t > > + #:use-module (srfi srfi-9) > > + #:use-module ((srfi srfi-43) > > + #:select (vector->list)) > > + #:use-module ((srfi srfi-69) #:prefix srfi69:) > > + #:use-module ((rnrs hashtables) #:prefix rnrs:) > > + #:use-module (srfi srfi-171 meta) > > + #:export (rcons reverse-rcons > > + rcount > > + rany >=20 > Better to put rcons on its own line so that other exports are also > aligned with the open paren. >=20 > > +;; A special value to be used as a placeholder where no value has b= een set and #f > > +;; doesn't cut it. Not exported. > > + > > +(define-record-type > > + (make-nothing) > > + nothing?) > > +(define nothing (make-nothing)) >=20 > Note that this can be somewhat cheaper as: >=20 > (define nothing (list 'nothing)) > (define (nothing? x) (eq? x nothing)) >=20 > > +;; helper function which ensures x is reduced. >=20 > Capitalize. FWIW, better done as a docstring: >=20 > (define (ensure-reduced x) > "Ensure that @var{x} is reduced." > ...) >=20 > > +;; helper function that wraps a reduced value twice since reducing = functions (like list-reduce) > > +;; unwraps them. tconcatenate is a good example: it re-uses it's re= ducer on it's input using list-reduce. > > +;; If that reduction finishes early and returns a reduced value, li= st-reduce would "unreduce" > > +;; that value and try to continue the transducing process. >=20 > Capitalize and limit to 80 characters wide. >=20 > > +(define (preserving-reduced f) > > + (lambda (a b) > > + (let ((return (f a b))) > > + (if (reduced? return) > > + (reduced return) > > + return)))) > > + > > + > > + > > + >=20 > Generally, put one blank line between functions. Two lines can be > between sections. Four is too much :) >=20 > > +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;= ; > > +;; Reducing functions meant to be used at the end at the transducin= g > > +;; process. ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;= ; >=20 > This is a fairly non-standard comment style, FWIW; consider just > prefixing with ";;;". >=20 > > +;; a transducer-friendly cons with the empty list as identity > > +(define rcons > > + (case-lambda >=20 > Similar comment regarding docstrings >=20 > > +;; Use this as the f in transduce to count the amount of elements p= assed through. > > +;; (transduce (tfilter odd?) tcount (list 1 2 3)) =3D> 2 >=20 > 80 characters, and the example can go in an @example if you like: >=20 > (define rcount > (case-lambda > "A transducer that counts the number of elements passing through= . \ > @example > (transduce (tfilter odd?) tcount (list 1 2 3)) @result{} 2 > @end example" > ...)) >=20 > > +(define (make-replacer map) > > + (cond > > + ((list? map) > > + (lambda (x) > > + (let ((replacer? (assoc x map))) > > + (if replacer? > > + (cdr replacer?) > > + x)))) >=20 > I generally find this sort of thing better with (ice-9 match): >=20 > (match (assoc x map) > ((x . replacer) replacer) > (#f x)) >=20 > > +;; Flattens everything and passes each value through the reducer > > +;; (list-transduce tflatten conj (list 1 2 (list 3 4 '(5 6) 7 8))) = =3D> (1 2 3 4 5 6 7 8) >=20 > 80 chars >=20 > > +;; I am not sure about the correctness of this. It seems to work. > > +;; we could maybe make it faster? > > +(define (tpartition f) >=20 > How could you know about the correctness? Probably a good idea to do > what it takes to be sure and then remove the comment. Regarding speed= , > I would remove the comment, if it's slow then people can work on it. >=20 > Note that in general comments shouldn't be from a first-person > perspective, because the code will be maintained as part of Guile, not= > necessarily by the author. >=20 > > diff --git a/module/srfi/srfi-171/gnu.scm b/module/srfi/srfi-171/gnu= .scm > > new file mode 100644 > > index 000000000..9aa8ab28e > > --- /dev/null > > +++ b/module/srfi/srfi-171/gnu.scm > > @@ -0,0 +1,49 @@ > > +(define-module (srfi srfi-171 gnu) >=20 > Needs a copyright header >=20 > > diff --git a/module/srfi/srfi-171/meta.scm b/module/srfi/srfi-171/me= ta.scm > > new file mode 100644 > > index 000000000..dd1fd06c4 > > --- /dev/null > > +++ b/module/srfi/srfi-171/meta.scm > > @@ -0,0 +1,115 @@ > > +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;= ;;;;;;;;;;;;;; > > +;; Copyright 2019 Linus Bj.rnstam >=20 > Probably should be a Guile copyright header > > diff --git a/test-suite/tests/srfi-171.test b/test-suite/tests/srfi-= 171.test > > new file mode 100644 > > index 000000000..c6d574af2 > > --- /dev/null > > +++ b/test-suite/tests/srfi-171.test > > @@ -0,0 +1,195 @@ > > +;; TODO: test all transducers that take an equality predicate > > +;; TODO: test treplace with all kinds of hash tables > > + > > +(define-module (test-srfi-171) >=20 > Needs a copyright header >=20 > > From 39be4808f5921a716916de6f4db03990412f2518 Mon Sep 17 00:00:00 20= 01 > > From: =3D?UTF-8?q?Linus=3D20Bj=3DC3=3DB6rnstam?=3D > > Date: Sun, 22 Dec 2019 15:39:35 +0100 > > Subject: [PATCH 2/2] Added documentation and tests for srfi-171. > > > > * doc/ref/srfi-modules.texi - Adapted and added the srfi document to= the > > guile srfi documentation > > * module/Makefile.am - Added the srfi files for compilation > > * test-suite/Makefile.am - Added the srfi-171.test to the test suite= . >=20 > Thanks for the per-file notes :) >=20 > FWIW the general format would be like: >=20 > * doc/ref/srfi-modules.texi: > * module/Makefile.am (SOURCES): > * test-suite/Makefile.am (SCM_TESTS): Add srfi-171. >=20 > I.e. colons after the file, and the changed function or variable in > parens, and then the tense is present: it describes the change. See > https://www.gnu.org/prep/standards/html_node/Change-Logs.html (though = we > adapt it for Git). >=20 > > +@node SRFI-171 > > +@subsection Transducers > > +@cindex SRFI-171 > > +@cindex transducers > > + > > +Some of the most common operations used in the Scheme language are = those transforming lists: map, filter, take and so on. They work well, a= re well understood, and are used daily by most Scheme programmers. They = are however not general because they only work on lists, and they do not= compose very well since combining N of them builds @code{(- N 1)} inter= mediate lists. >=20 > Limit to 80 chars please. In Emacs you'd do M-q to do this. >=20 > > +@itemize @bullet >=20 > Probably you can leave off @bullet. >=20 > > +@example > > +(list-transduce > > + (tmap (lambda (x) (+ x 1))) > > + (revery (lambda (v) (if (odd? v) v #f))) > > + (list 2 4 6)) @result{} 7 > > + > > + > > +(list-transduce (tmap (lambda (x) (+ x 1)) (revery odd?) (list 2 4 = 5 6)) @result{} #f > > +@end example >=20 > Probably better style to have just one empty line. I would also break= > the last line. >=20 > Cheers, >=20 > Andy >