[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Add srfi-171 to guile

From: Linus Björnstam
Subject: Re: [PATCH] Add srfi-171 to guile
Date: Thu, 16 Jan 2020 20:52:33 +0100
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

I have addressed all your feedback and present to you the revised patch. Some small code changes compared to the other patch. Most significantly, tsegment and tpartition have gotten paranoid about the downstream reducer managing to sneak a reduced value through (with no performance impact, mind you). Apart from that, some docstrings were added where it seemed sane, some accidental code duplication between (srfi srfi-171) and (srfi srfi-171 meta) was fixed.

The only thing I am scared of is thunderbird accidentally sending this mail as HTML and maybe getting the commit message formatting wrong.

Best regards

Linus Björnstam

On 2020-01-05 12:30, Andy Wingo wrote:
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örnstam <address@hidden> writes:

 From 7e8d3b22ba5f814c40dbb5ab616a318c0cdc2f3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Linus=20Bj=C3=B6rnstam?= <address@hidden>
Date: Sun, 22 Dec 2019 15:38:34 +0100
Subject: [PATCH 1/2] Added srfi-171 to guile under the module name (srfi

For more info, read the SRFI document:
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 course
+;; (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

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,

+(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 set and 
+;; doesn't cut it. Not exported.
+(define-record-type <nothing>
+  (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 functions 
(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-reduce would 
+;; 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 
+;; (transduce (tfilter odd?) tcount (list 1 2 3)) => 2
80 characters, and the example can go in an @example if you like:

   (define rcount
       "A transducer that counts the number of elements passing through. \
(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))) => (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.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)
Needs a copyright header

 From 39be4808f5921a716916de6f4db03990412f2518 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Linus=20Bj=C3=B6rnstam?= <address@hidden>
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/ - Added the srfi files for compilation
* test-suite/ - 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/ (SOURCES):
   * test-suite/ (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 (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.

+  (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.



 - Linus Björnstam

Attachment: 0001-Add-SRFI-171-transducers-to-guile.patch
Description: Text document

reply via email to

[Prev in Thread] Current Thread [Next in Thread]