Re: Generators (iterators) for Gnu Emacs

[Stefan Monnier <monnier@iro.umontreal.ca>]

> I want to contribute a package about generators to Gnu Emacs.

Thanks.  Looks good.  I generally prefer adding packages to GNU ELPA,
but I guess it also makes sense to add this directly to Emacs.

> Daniel Colascione suggested to me to rename the thing to "iterators.el",
> since this package doesn't use continuation passing style to implement
> "yield".  I guess that's reasonable.

I don't have any opinion on the naming.  A few comments below.


        Stefan


> ;;; Commentary:

Please state clearly which are all the basic operations one can perform on
a generator (AFAICT there's only one such operation, which is `gen-next').

It's great to see that you haven't needed anything else than `gen-next'.
I'd have expected a `gen-done-p' test to be needed/handy every once in
a while.

> (defmacro gen-make (&rest body)
>   "Return a generator that evaluates BODY to generate elements.
> For each call of `gen-next' with the the returned generator, BODY
> will be evaluated to produce an element."
>   (let ((this-element (make-symbol "this-element")))
>     `(let (,this-element)
>        (lambda ()

AFAICT this requires lexical-binding in the caller, so we might want to
signal an error if lexical-binding is nil.

>          (if (eq ,this-element 'gen-done)
>              'gen-done
>            (setq ,this-element (progn ,@body)))))))

This code breaks down if your generator happens to return the symbol
`gen-done'.  A better option is to have a (defconst gen-done <val>)
where the value is unique w.r.t `eq' (can be a string, a cons cell, an
uninterned symbol, you name it) and then to use this value rather than
hard-coding an interned symbol.

> (defun gen-next (generator)
>   (funcall generator))

A defalias would be more efficient.

>    (if (not (null elements))

Aka (if elements

> The sequence of returned elements is starting with VALUE. Any
                                                          ^^^
Add extra space (or break the line).

> successive element will be found by calling FUNCTION on the
> preceding element."
>   (gen-append
>    (gen-from-elts value)
>    (gen-make (setq value (funcall function value)))))

I guess a `gen-cons' would be more efficient than this 
(gen-append (gen-from-elts value) ...).

> (defun gen-number-range (&optional start end inc)
>   "Return a generator of a number sequence.
> START denotes the first number and defaults to 1.

Tradition in computer science is to start counting from 0.  There are
some notable exceptions (such as `point-min', which I regret), but
I strongly recommend to follow the tradition.

> The second optional argument END specifies the upper limit.  If nil,
> the returned generator is infinite.  INC is the increment used between
> numbers and defaults to 1."

Please be more precise about whether END is included or excluded.
Tradition (see again) is to exclude the upper bound (e.g. in dotimes).

> (defmacro gen-delay-expr (expression)

Have you made use of this?  I'm not sure it really fits.  I mean,
technically it works, but I'm not completely sure if pretending it's
a generator is a clever idea or not.

>   (let ((done nil) el)
>     (gen-make
>      (if done 'gen-done
>        (setq el (gen-next generator))
>        (while (and (not (eq el 'gen-done))
>                    (not (funcall predicate el)))
>          (setq el (gen-next generator)))
>        (if (eq el 'gen-done)
>            (setq done 'gen-done)
>          el)))))

Better move the `el' into the `gen-make' so it's not caught as a free
variable of the closure.  And please apply de-Morgan's law.

>     (let ((current (pop generators)) el (done nil) (skip (make-symbol "skip")))
>       (gen-delq
>        skip
>        (gen-make 
>         (if done 'gen-done
>           (setq el (gen-next current))
>           (if (eq el 'gen-done)
>               (if (null generators)
>                   (progn (setq done t) 'gen-done)
>                 (setq current (pop generators))
>                 skip)
>             el)))))))

Same here: sink `el' into the `gen-make'.

>   (nreverse (gen-reduce (lambda (x y) (cons y x)) () generator)))

Too bad the order of arguments was not swapped so we could use #'cons
instead of this lambda.

>   "Return true if PREDICATE is true for any element of GENERATOR."
            ^^^^
We call it "non-nil".  Same for `gen-every'.

>   (cl-callf or function #'identity)
>   (gen-max generator (and function (lambda (x) (- (funcall function x))))))

AFAICT, the first line makes sure `function' is not nil, so you can
remove the "(and function" on the second line.