Re: [PATCH] add SRFI-119 / language/wisp to Guile? (new patch, squashed)

[Ludovic Courtès]

Hello Arne,

"Dr. Arne Babenhauserheide" <arne_bab@web.de> skribis:

> I did the changes for the review. It took a while (sorry for that) but
> it got cleaner as a result (thank you!)
>
> Firstoff: I’d like to assign my copyright to the FSF. I’ll sign the
> papers once I receive them. Also I have an Employer disclaimer of rights
> on paper for Emacs already, so that should not cause trouble.

OK.  I assumed you already emailed assign@gnu.org the relevant form,
right?  Let us know how it goes.

> Changes:
>
> - [X] LGPL
> - [X] Please add the new files to the relevant ‘Makefile.am’ files.
> - [X] Note the changes in Makefiles in the commit
> - [X] Please capitalize “Scheme” and “Wisp” (in general we like to pay 
> attention to typography, spelling, and language in the manual.)
> - [X] s/(wisp)/, also referred to as @dfn{Wisp} (for ``Whitespace …'')/
> - [X] Two spaces after end-of-sentence periods, to facilitate navigation in 
> Emacs.
> - [X] indent “the usual way”
> - [X] comments always with ;; except for margin comments
> - [X] (read-enable 'curly-infix)
>   This needs to be:
>     (eval-when (expand load eval)
>       (read-enable 'curly-infix))
> - [X] Please make them #:use-module clauses in the ‘define-module’ form
> - [X] I’d encourage following the usual naming convention, so
>   ‘in-indent?’, ‘in-comment?’, etc.
> - [X] use exception objects or SRFI-35 error conditions instead of throw with 
> symbols
> - [X] add a test for source location info

Awesome.

> A change I did not do:
>
> - [ ] +Use record-types for the lines+. Reason: I decided not to do
>   this because it currently needs to propagate the source properties
>   when retrieving the code, so this is not a good match for a record
>   type (it may become one with an annotated reader, but I’d like to
>   shift that to a later change:

What about having a ‘location’ field in that record type?  Would that
work for you?  Or, alternatively, add source properties just on the
relevant part of the list.

Having lots of ‘car’ and ‘cdr’ in the code to access the various
“fields” of the line hinders readability and prevents proper
type-checking and error-reporting.

As I wrote back in June, source properties are not ideal and explicitly
storing location info, as is done in syntax objects, is preferable:

  https://lists.gnu.org/archive/html/guile-devel/2023-06/msg00008.html

Some comments:

> From 5857f8b961562d1ad2ae201401d5343233422eff Mon Sep 17 00:00:00 2001
> From: Arne Babenhauserheide <arne_bab@web.de>
> Date: Fri, 3 Feb 2023 22:20:04 +0100
> Subject: [PATCH] Add language/wisp, wisp tests, and srfi-119 documentation
>
> * doc/ref/srfi-modules.texi (srfi-119): add node
> * module/language/wisp.scm: New file.
> * module/language/wisp/spec.scm: New file.
> * test-suite/tests/srfi-119.test: New file.
> * am/bootstrap.am (SOURCES): add language/wisp.scm and language/wisp/spec.scm
> * test-suite/Makefile.am (SCM_TESTS): add tests/srfi-119.test

[...]

> +(define wisp-uuid "e749c73d-c826-47e2-a798-c16c13cb89dd")
> +;; define an intermediate dot replacement with UUID to avoid clashes.
> +(define repr-dot ; .
> +  (string->symbol (string-append "REPR-DOT-" wisp-uuid)))

As I wrote in June, please remove those UUIDs and use uninterned symbols
instead (which cannot be serialized but can be compared with ‘eq?’,
which is all we need.)

> +(define (match-charlist-to-repr charlist)
> +  (let
> +      ((chlist (reverse charlist)))
> +    (cond
> +     ((equal? chlist (list #\.))
> +      repr-dot)
> +     ((equal? chlist (list #\'))
> +      repr-quote)

This would probably be more readable with ‘match’ instead of ‘cond’.

Also, as mentioned regarding the naming convention, please write
‘char-list’ or ‘chars’ rather than ‘chlist’.

> +(define (wisp-read port)
> +  "wrap read to catch list prefixes."
      ^
Capital letter.  Also maybe mention PORT and the return value.

> +      (cond
> +       ((or (< prefix-maxlen (length peeked)) (eof-object? (peek-char port)) 
> (equal? #\space (peek-char port)) (equal? #\newline (peek-char port)))
> +        (if repr-symbol ; found a special symbol, return it.
> +            repr-symbol
> +            (let unpeek
> +                ((remaining peeked))

Please avoid long lines and write ‘let’ on a line as in:

  (let ((x 2))
    …)

or:

  (let loop ((x 1))
    …)

(This is the style commonly used elsewhere in Guile.)

> +(define (line-continues? line)
> +  (equal? repr-dot (car (line-code line))))
> +
> +(define (line-only-colon? line)
> +  (and
> +   (equal? ":" (car (line-code line)))
> +   (null? (cdr (line-code line)))))
> +
> +(define (line-empty-code? line)
> +  (null? (line-code line)))

I think this illustrates excessive use of lists, car, and cdr, and its
impact on readability.

> +(define (with-read-options opts thunk)
> +  (let ((saved-options (read-options)))
> +    (dynamic-wind
> +        (lambda ()
> +          (read-options opts))
> +        thunk
> +        (lambda ()
> +          (read-options saved-options)))))
> +
> +(define (wisp->list str)
> +        (wisp-scheme-read-string str))

Please reindent (M-q) these two procedures.  :-)

> +(with-test-prefix "wisp-read-simple"
> +  (pass-if (equal? (wisp->list "<= n 5")    '((<= n 5))))
> +  (pass-if (equal? (wisp->list ". 5") '(5)))
> +  (pass-if (equal? (wisp->list "+ 1 : * 2 3") '((+ 1 (* 2 3))))))

Prefer ‘pass-if-equal’:

  (pass-if-equal '((<= n 5))
    (wisp->list "<= n 5"))

That gives better reporting in ‘check-guile.log’.

> +(with-test-prefix "wisp-source-properties"
> +  (pass-if (not (find null? (map source-properties (wisp->list "1 . 2\n3 4\n 
>   5 . 6")))))
> +  (pass-if (not (find null? (map source-properties (wisp->list "1 2\n3 4\n   
> 5 6"))))))

Maybe avoid the double negation by writing: (every pair? (map …)).

However, why not check explicitly the line/column numbers?  It seems to
me like this would be more appropriate: we want to make sure the Wisp
parser gets it right so users can get accurate error reporting.

That’s it!  I’m hope I’m not adding too much to what I already wrote in
the previous review.  Let me know!

Thank you,
Ludo’.