unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
From: "Linus Björnstam" <linus.bjornstam@veryfast.biz>
To: "Andy Wingo" <wingo@pobox.com>
Cc: guile-devel <guile-devel@gnu.org>
Subject: Re: [PATCH] Add srfi-171 to guile
Date: Sun, 05 Jan 2020 14:34:35 +0100	[thread overview]
Message-ID: <ac2e476e-e764-4520-8882-329da5f0560a@www.fastmail.com> (raw)
In-Reply-To: <87mub2ywcc.fsf@pobox.com>

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 transducers worked. I know how to verify and test that they work as they should. 

Regarding the copyright block I was a bit too trigger happy. I completely forgot!

I have loads at work right now (2 weeks of modern music cd recording. Fun times!), but so will make sure to try to get everything done before the end of January.

Best regards
  Linus Björnstam

On Sun, 5 Jan 2020, at 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 <linus.bjornstam@veryfast.biz> writes:
> 
> > From 7e8d3b22ba5f814c40dbb5ab616a318c0cdc2f3e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Linus=20Bj=C3=B6rnstam?= <linus.bjornstam@fastmail.se>
> > 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 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
> 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 set and #f
> > +;; 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 "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)) => 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))) => (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?= <linus.bjornstam@fastmail.se>
> > 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
>



  reply	other threads:[~2020-01-05 13:34 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-22 14:55 [PATCH] Add srfi-171 to guile Linus Björnstam
2019-12-22 20:45 ` Linus Björnstam
2020-01-05 11:30 ` Andy Wingo
2020-01-05 13:34   ` Linus Björnstam [this message]
2020-01-16 19:52   ` Linus Björnstam
2020-03-08 14:40     ` Ludovic Courtès
2020-03-08 17:11       ` Linus Björnstam
2020-03-23 17:17       ` Linus Björnstam
2020-03-25 21:48         ` Ludovic Courtès

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ac2e476e-e764-4520-8882-329da5f0560a@www.fastmail.com \
    --to=linus.bjornstam@veryfast.biz \
    --cc=guile-devel@gnu.org \
    --cc=wingo@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).