From mboxrd@z Thu Jan  1 00:00:00 1970
Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail
From: =?utf-8?Q?Ludovic_Court=C3=A8s?= <ludo@gnu.org>
Newsgroups: gmane.lisp.guile.devel
Subject: Re: [PATCH] add SRFI-119 / language/wisp to Guile? (new patch,
 squashed)
Date: Fri, 18 Aug 2023 12:29:42 +0200
Message-ID: <87il9ctzhl.fsf@gnu.org>
References: <87h6w2fkz8.fsf@web.de> <877cwxe4ar.fsf@web.de>
 <2f7d015d-ceb4-ef8f-b4fe-b69e39b723f8@telenet.be>
 <87357ldqaq.fsf@web.de>
 <1a70460e-11fb-9f5d-0d5f-1eb507d5af0d@telenet.be>
 <87ilg4j65e.fsf@web.de> <87edqsj5vt.fsf@web.de>
 <01212259-37dd-5d67-7bbc-101e01d96d01@telenet.be>
 <1a6c8dda-0124-124c-f932-937a11386ced@gmail.com>
 <87fsb5i912.fsf@web.de>
 <c2ae22ea-fd38-f44c-c7d2-315d0b36a933@gmail.com>
 <87ttzc7gwa.fsf@gnu.org>
 <1e0d07bc-dcf8-fe56-7f16-a72e5df0c20d@telenet.be>
 <875ybr2hk9.fsf@gnu.org> <87v8jrdmk5.fsf@web.de>
 <87jzzr7cba.fsf@web.de> <87v8hc8i8v.fsf@web.de>
 <87legrs23a.fsf@gnu.org>
 <209e68fd-b010-8213-6c9b-a0d1b8f0f72c@telenet.be>
 <87o7jf2slw.fsf@web.de> <875y5h8j04.fsf@web.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214";
	logging-data="3381"; mail-complaints-to="usenet@ciao.gmane.io"
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Cc: guile-devel@gnu.org
To: "Dr. Arne Babenhauserheide" <arne_bab@web.de>
Original-X-From: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Fri Aug 18 12:33:57 2023
Return-path: <guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org>
Envelope-to: guile-devel@m.gmane-mx.org
Original-Received: from lists.gnu.org ([209.51.188.17])
	by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org>)
	id 1qWwnk-0000ch-Uv
	for guile-devel@m.gmane-mx.org; Fri, 18 Aug 2023 12:33:57 +0200
Original-Received: from localhost ([::1] helo=lists1p.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.90_1)
	(envelope-from <guile-devel-bounces@gnu.org>)
	id 1qWwnM-00066J-H1; Fri, 18 Aug 2023 06:33:32 -0400
Original-Received: from eggs.gnu.org ([2001:470:142:3::10])
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ludo@gnu.org>) id 1qWwjj-0005fc-MQ
 for guile-devel@gnu.org; Fri, 18 Aug 2023 06:29:47 -0400
Original-Received: from fencepost.gnu.org ([2001:470:142:3::e])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <ludo@gnu.org>)
 id 1qWwji-0001Uc-M2; Fri, 18 Aug 2023 06:29:46 -0400
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org;
 s=fencepost-gnu-org; h=MIME-Version:In-Reply-To:Date:References:Subject:To:
 From; bh=aJm5yz/zKHizDfIn5wIH74Un2XXgAyypI77r3DFwEzM=; b=NcNE9kQ5l5J2ksgkjc/K
 CzvsFXdEc+xH/+tbbQMYG8MOvIuU1sZ1ZcZWTQQzMJS5SRWp61yEVjm3EumSpgkUz9UV1DEj4sx1R
 LRZJVRCFX4fD4syXgSvXmtf/dosgvD3vfIFPucEJVyiDw2K0jd1TdOl7C3gO45N8CM6NnfOl/y04e
 PWfzeMm5X5eKEa6by8OwxnPhXgD4yKkG73lMF56FmGJt+DVJwDtk30VhcRhcjwEUPYAv4/fRVDpF8
 rz2Ip6EWx8n4568FBxhyMo7BGksjcWdAsrB7Tk0aUsK+5XWed5agmxFMhIVJi4g+k8t/EUTB951dS
 y40UOEfXyEDH2A==;
X-URL: http://www.fdn.fr/~lcourtes/
X-Revolutionary-Date: Primidi 1 Fructidor an 231 de la =?utf-8?Q?R=C3=A9vo?=
 =?utf-8?Q?lution=2C?= jour de la Prune
X-PGP-Key-ID: 0x090B11993D9AEBB5
X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc
X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4  0CFB 090B 1199 3D9A EBB5
X-OS: x86_64-pc-linux-gnu
In-Reply-To: <875y5h8j04.fsf@web.de> (Arne Babenhauserheide's message of "Mon, 
 14 Aug 2023 22:11:55 +0200")
X-BeenThere: guile-devel@gnu.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: "Developers list for Guile,
 the GNU extensibility library" <guile-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guile-devel>,
 <mailto:guile-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/guile-devel>
List-Post: <mailto:guile-devel@gnu.org>
List-Help: <mailto:guile-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guile-devel>,
 <mailto:guile-devel-request@gnu.org?subject=subscribe>
Errors-To: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org
Original-Sender: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org
Xref: news.gmane.io gmane.lisp.guile.devel:21916
Archived-At: <http://permalink.gmane.org/gmane.lisp.guile.devel/21916>

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=E2=80=99d like to assign my copyright to the FSF. I=E2=80=99l=
l 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 =E2=80=98Makefile.am=E2=80=
=99 files.
> - [X] Note the changes in Makefiles in the commit
> - [X] Please capitalize =E2=80=9CScheme=E2=80=9D and =E2=80=9CWisp=E2=80=
=9D (in general we like to pay attention to typography, spelling, and langu=
age in the manual.)
> - [X] s/(wisp)/, also referred to as @dfn{Wisp} (for ``Whitespace =E2=80=
=A6'')/
> - [X] Two spaces after end-of-sentence periods, to facilitate navigation =
in Emacs.
> - [X] indent =E2=80=9Cthe usual way=E2=80=9D
> - [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 =E2=80=98define-module=
=E2=80=99 form
> - [X] I=E2=80=99d encourage following the usual naming convention, so
>   =E2=80=98in-indent?=E2=80=99, =E2=80=98in-comment?=E2=80=99, 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=E2=80=99d like =
to
>   shift that to a later change:

What about having a =E2=80=98location=E2=80=99 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 =E2=80=98car=E2=80=99 and =E2=80=98cdr=E2=80=99 in the code =
to access the various
=E2=80=9Cfields=E2=80=9D 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 =E2=80=98eq?=
=E2=80=99,
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 =E2=80=98match=E2=80=99 instead o=
f =E2=80=98cond=E2=80=99.

Also, as mentioned regarding the naming convention, please write
=E2=80=98char-list=E2=80=99 or =E2=80=98chars=E2=80=99 rather than =E2=80=
=98chlist=E2=80=99.

> +(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 po=
rt)) (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 =E2=80=98let=E2=80=99 on a line as in:

  (let ((x 2))
    =E2=80=A6)

or:

  (let loop ((x 1))
    =E2=80=A6)

(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 "<=3D n 5")    '((<=3D n 5))))
> +  (pass-if (equal? (wisp->list ". 5") '(5)))
> +  (pass-if (equal? (wisp->list "+ 1 : * 2 3") '((+ 1 (* 2 3))))))

Prefer =E2=80=98pass-if-equal=E2=80=99:

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

That gives better reporting in =E2=80=98check-guile.log=E2=80=99.

> +(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 =E2=80=A6)).

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=E2=80=99s it!  I=E2=80=99m hope I=E2=80=99m not adding too much to wha=
t I already wrote in
the previous review.  Let me know!

Thank you,
Ludo=E2=80=99.