Hello Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

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

If I read it right, the docs
https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html
say that I need to ask someone to get the papers I can then sign.

Can I get the papers from somewhere or does someone have to send them to
me?

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

I did part of the refactoring to a record type but it became less clear
to read than working on the list, because the special cases to handle
increased, so I reverted the change.

It really didn’t get cleaner.

car and cdr are mostly used in the variable length code-part of the line.

> 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

Once I get that sorted out, that would be the moment I’d switch to
records (then they would need no added special casing), but I would
prefer to take your offer to do that change in a followup patch.

> 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.)

I already tried this when Maxime suggested it, but it did not work. I no
longer remember in which part of the code it broke, but it broke, so I
reverted to using the uuids.

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

Match was too clever here. But changed formatting already gets close,
and a helper ges further:

(define (equal-rest? chars . args)
  (equal? chars args))

(define (match-charlist-to-repr char-list)
  (let ((chars (reverse char-list)))
    (cond
     ((equal-rest? chars #\.) repr-dot)
     ((equal-rest? chars #\') repr-quote)
     ((equal-rest? chars #\,) repr-unquote)
     ((equal-rest? chars #\`) repr-quasiquote)
     ((equal-rest? chars #\, #\@) repr-unquote-splicing)
     ((equal-rest? chars #\# #\') repr-syntax)
     ((equal-rest? chars #\# #\,) repr-unsyntax)
     ((equal-rest? chars #\# #\`) repr-quasisyntax)
     ((equal-rest? chars #\# #\, #\@) repr-unsyntax-splicing)
     (else #f))))


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

Fixed — thank you!

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

Good point — the return value is pretty important here. New doc:

"Wrap read to catch list prefixes: read one or several chars from PORT and return read symbols or replacement-symbols as representation for special forms."

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

Much nicer — thank you!

  (let ((prefix-maxlen 4))
    (let longpeek ((peeked '()) (repr-symbol #f))
      (cond
       ((or (< prefix-maxlen (length peeked))
            (eof-object? (peek-char port))
            (equal? #\space (peek-char port))
            (equal? #\newline (peek-char port)))


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

This is a place where the line has variable length, so I can’t make it
nicer with a record type without adding more complexity.

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

Ah, missed running indent-region on these. Thank you!
(now I re-indented the whole test file)

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

That’s much nicer! Thank you!

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

That’s beautiful! I had thought about a way to do this but didn’t think
about pair?.

But actually what we need is that the source-properties are the same
from Wisp and from Scheme. And that’s actually something else than what
I did (though I thought that I did it), so this needed a few changes.

Previously it reported as line the *end* of a form, but Guile usually
reports the beginning of a form. I now changed the parser to also report
the beginning of a form.

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

I hope the new patch is ready to merge.

I’m attaching the new squashed patch again here and will add the patches
for the review changes to a second email.