From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Add srfi-171 to guile Date: Sun, 05 Jan 2020 12:30:11 +0100 Message-ID: <87mub2ywcc.fsf@pobox.com> References: 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="142874"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) Cc: guile-devel To: Linus =?utf-8?Q?Bj=C3=B6rnstam?= Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Sun Jan 05 12:30:37 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 1io47B-000b2p-1q for guile-devel@m.gmane.org; Sun, 05 Jan 2020 12:30:37 +0100 Original-Received: from localhost ([::1]:41270 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1io479-0007hj-T4 for guile-devel@m.gmane.org; Sun, 05 Jan 2020 06:30:35 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:49857) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1io472-0007f4-Cm for guile-devel@gnu.org; Sun, 05 Jan 2020 06:30:31 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1io470-0006Ma-65 for guile-devel@gnu.org; Sun, 05 Jan 2020 06:30:28 -0500 Original-Received: from fanzine.igalia.com ([178.60.130.6]:60049) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1io46z-0006Cj-IJ for guile-devel@gnu.org; Sun, 05 Jan 2020 06:30:26 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:MIME-Version:Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From; bh=RAgP8dP3hPmdho+aKYTIjBwFpnExoDp0EuW1HrpLlO8=; b=ICzodAm1cJ8nKYHnG3PtgknZLPkFaXryp+ewTQ149VGpxem4EzGdTb7uw2p5CwMt6A8KUodOs7RIUNahzPWUWuk6b2tGFoKDgpe2a5ChzqOw+AW9Efj1eCAEjsgOwoDqD3/uhytLTS3WsvzBHvHj0oNPjorYdf3xfBV/Hl+U4KNiB+z+GpEM+leOBiVvS/IHpzq8bKtkAGCRiWVPJczFST3jTYzwncCz5VSzxjapzTW/8waA3ZrxXeBzf5YZ2wjS5mWfajVd0vdd+p7iZyd+CfppYUGSAgzUjQjITlam8SrvJxVd9IaT+1tE4RkEtnmwWVEgiICO4AWSugMc1jBu+A==; Original-Received: from [88.123.12.110] (helo=sparrow) by fanzine.igalia.com with esmtpsa (Cipher TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim) id 1io46v-0004TR-5o; Sun, 05 Jan 2020 12:30:21 +0100 In-Reply-To: ("Linus \=\?utf-8\?Q\?Bj\=C3\=B6rnstam\=22's\?\= message of "Sun, 22 Dec 2019 15:55:55 +0100") X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] [fuzzy] X-Received-From: 178.60.130.6 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:20218 Archived-At: Hi :) 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. On Sun 22 Dec 2019 15:55, Linus Bj=C3=B6rnstam writes: > From 7e8d3b22ba5f814c40dbb5ab616a318c0cdc2f3e Mon Sep 17 00:00:00 2001 > 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/srfi-171= /srfi-171.html Needs a note per-file; see other commit log messages. Also please wrap to 72 characters. > --- /dev/null > +++ b/module/srfi/srfi-171.scm > @@ -0,0 +1,498 @@ > +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;= ;;;;;;;;; > +;; Copyright 2019 Linus Bj.rnstam > +;; I think you've assigned copyright so this can have the standard Guile copyright block, right? > +;; This module name is guile-specific. The correct module name is of cou= rse > +;; (srfi 171) 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. The style in Guile is generally that block comments like this should be complete sentences, starting with capital letters and including terminating punctuation. Generally we do two spaces after periods, also. > +(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 Better to put rcons on its own line so that other exports are also aligned with the open paren. > +;; A special value to be used as a placeholder where no value has been s= et and #f > +;; doesn't cut it. Not exported. > + > +(define-record-type > + (make-nothing) > + nothing?) > +(define nothing (make-nothing)) Note that this can be somewhat cheaper as: (define nothing (list 'nothing)) (define (nothing? x) (eq? x nothing)) > +;; helper function which ensures x is reduced. Capitalize. FWIW, better done as a docstring: (define (ensure-reduced x) "Ensure that @var{x} is reduced." ...) > +;; helper function that wraps a reduced value twice since reducing funct= ions (like list-reduce) > +;; unwraps them. tconcatenate is a good example: it re-uses it's reducer= on it's input using list-reduce. > +;; If that reduction finishes early and returns a reduced value, list-re= duce would "unreduce" > +;; that value and try to continue the transducing process. Capitalize and limit to 80 characters wide. > +(define (preserving-reduced f) > + (lambda (a b) > + (let ((return (f a b))) > + (if (reduced? return) > + (reduced return) > + return)))) > + > + > + > + Generally, put one blank line between functions. Two lines can be between sections. Four is too much :) > +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; > +;; Reducing functions meant to be used at the end at the transducing > +;; process. ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; This is a fairly non-standard comment style, FWIW; consider just prefixing with ";;;". > +;; a transducer-friendly cons with the empty list as identity > +(define rcons > + (case-lambda Similar comment regarding docstrings > +;; Use this as the f in transduce to count the amount of elements passed= through. > +;; (transduce (tfilter odd?) tcount (list 1 2 3)) =3D> 2 80 characters, and the example can go in an @example if you like: (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" ...)) > +(define (make-replacer map) > + (cond > + ((list? map) > + (lambda (x) > + (let ((replacer? (assoc x map))) > + (if replacer? > + (cdr replacer?) > + x)))) I generally find this sort of thing better with (ice-9 match): (match (assoc x map) ((x . replacer) replacer) (#f x)) > +;; 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) 80 chars > +;; I am not sure about the correctness of this. It seems to work. > +;; we could maybe make it faster? > +(define (tpartition f) 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. 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. > 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) Needs a copyright header > diff --git a/module/srfi/srfi-171/meta.scm b/module/srfi/srfi-171/meta.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 Probably should be a Guile copyright header > diff --git a/test-suite/tests/srfi-171.test b/test-suite/tests/srfi-171.t= est > 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) Needs a copyright header > From 39be4808f5921a716916de6f4db03990412f2518 Mon Sep 17 00:00:00 2001 > 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. Thanks for the per-file notes :) FWIW the general format would be like: * doc/ref/srfi-modules.texi: * module/Makefile.am (SOURCES): * test-suite/Makefile.am (SCM_TESTS): Add srfi-171. 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). > +@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, are 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)} intermediate lists. Limit to 80 chars please. In Emacs you'd do M-q to do this. > +@itemize @bullet Probably you can leave off @bullet. > +@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 Probably better style to have just one empty line. I would also break the last line. Cheers, Andy