Re: reading job-specifier.scm

[Dale Mellor]

On Thu, 2022-01-13 at 21:46 +0800, Wensheng Xie wrote:
> Hi,
> 
> when reading the code of mcron, for example, the file job-
> specifier.scm, I noted that the following code
> 
> ```
> (define* (range start end #:optional (step 1))
>   "Produces a list of values from START up to (but not including) END.
> An
> optional STEP may be supplied, and (if positive) only every step'th
> value will
> go into the list.  For example, (range 1 6 2) returns '(1 3 5)."
>   (let ((step* (max step 1)))
>     (unfold (λ (i) (>= i end))          ;predicate
>             identity                    ;value
>             (λ (i) (+ step* i))         ;next seed
>             start)))                    ;seed
> ```
> 
> I want to understand what is the advantage here. Why don't you write it
> like
> 
> ```
> (define* (range start end #:optional (step 1))
>   "Produces a list of values from START up to (but not including) END.
> An
> optional STEP may be supplied, and (if positive) only every step'th
> value will
> go into the list.  For example, (range 1 6 2) returns '(1 3 5)."
>   (let ((step* (max step 1)))
>     (if (>= start end)
>         '()
>         (cons start (range (+ start step*) end step*))))
> ```
> 
> best regards,
> wxie



Hello Wensheng,

   the real 'problem' with your version, as with some of the earlier
versions of the code as seen below, is that the loop you have created is
not properly tail-recursive, which means that the final range call has to
involve stack manipulation and function call/return processing.



   You must understand that mcron is a twenty year old program these days
and has gone through some small paradigm shifts as Guile itself has
evolved and become more capable over the years.  In fact, the whole code
was unceremoniously ploughed-over in early 2016, and commit 5da002 changed
the procedure from

```
(define (range start end . step)
  (let ((step (if (or (null? step)
                      (<= (car step) 0))
                  1
                  (car step))))
    (let loop ((start start))
      (if (>= start end) '()
          (cons start
                (loop (+ start step)))))))
```

(which has the tail-recursion problem), to

```
(define* (range start end #:optional (step 1))
  "..."
  (unfold (cut >= <> end) identity (cute + <> (max step 1)) start))
```

which is arguably cleaner, although I really don't think it is; I find it
very hard to see at first glance how the logic of this works.  It does
solve the problem that there are no recursive function calls, though does
use a relatively large memory space instead.  This version uses many
constructs which were not available when the previous version was written
around 2003.

   The same committer changed the code again a short time later to the
current version.  It is telling that he had to include comments to explain
the call of the unfold function!



   Your version also has the unfortunate effect of unnecessarily calling
the max function multiple times when it is not needed, and would be better
written as

```
(define (range start end #:optional (step 1))
  "..."
  (let loop ((start start) (step (max step 1)))
     (if (>= start end)
        '()
        (cons start (loop (+ start step) step)))))
```

But if I was writing it today I would probably go with

```
(define (range start end #:optional (step 1))
   "..."
   (let ((step (max step 1)))
      (iota (/ (- end start) step) start step)))
```



   So you see it is all about time and fashion and the state of Guile
itself.  Speed is not really needed here, so anything which works, works.

   I had expected that one of the other maintainer/contributors to the
code base would have addressed this, but I hope I have been able to answer
your question,

Best wishes,
Dale

signature.asc
Description: This is a digitally signed message part