Re: [PATCH] Per-port read options, reader directives, SRFI-105

[ludo@gnu.org (Ludovic Courtès)]

Hi!

Regarding SRFI-105, I’m skeptical about a couple of things.

First, $bracket-apply$, $nfx$, and $bracket-list$ need to be
user-defined, but implementations are allowed to provide a pre-defined
version of these.  This sounds like an opportunity for incompatibilities
(which the document describes as a shortcoming of Guile’s infix module.)

It’s also unhygienic, in the sense that programs that need it would
typically have to start with a definition of $nfx$ & co., although these
identifiers never appear literally in the neoteric code.

Bracket lists (and $bracket-list$) are another opportunity for
incompatibilities, IMO.  The SRFI reads:

  several Scheme implementations follow the R6RS specification that
  accepts [...] as a synonym for (...), GNU Kawa interprets [...] as the
  redefinable constructor ($bracket-list$ ...)

But I fail to see why R6 syntax would influence SRFI-105 syntax.

That said...

Mark H Weaver <mhw@netris.org> skribis:

> From 3345a52824c2ae0e6ffe64ec7e07609d1bc362ef Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Wed, 24 Oct 2012 14:50:16 -0400
> Subject: [PATCH 3/3] Implement SRFI-105 curly infix expressions.
>
> * libguile/private-options.h: Add SCM_CURLY_INFIX_P macro, and increment
>   SCM_N_READ_OPTIONS.
>
> * libguile/read.c (sym_nfx, sym_bracket_list, sym_bracket_apply): New
>   symbols.
>   (scm_read_opts): Add curly-infix reader option.
>   (scm_t_read_opts): Add curly_infix_p and neoteric_p fields.
>   (init_read_options): Initialize new fields.
>   (CHAR_IS_DELIMITER): Add '{', '}', '[', and ']' as delimiters if
>   curly_infix_p is set.
>
>   (set_port_square_brackets_p, set_port_curly_infix_p): New functions.
>
>   (scm_read_expression_1): New internal static function, which contains
>   the code that was previously in 'scm_read_expression'.  Handle curly
>   braces when curly_infix_p is set.  If curly_infix_p is set and
>   square_brackets_p is unset, follow the Kawa convention:
>   [...] => ($bracket-list$ ...)
>
>   (scm_read_expression): New function body to handle neoteric
>   expressions where appropriate.
>
>   (scm_read_shebang): Handle the new reader directives: '#!curly-infix'
>   and the non-standard '#!curly-infix-and-bracket-lists'.
>
>   (scm_read_sexp): Handle curly infix lists.
>
> * module/ice-9/boot-9.scm (%cond-expand-features): Add srfi-105 feature
>   identifier.
>
> * doc/ref/srfi-modules.texi (SRFI-105): Add stub doc for SRFI-105.
>
> * doc/ref/api-evaluation.texi (Scheme Read): Add documentation for the
>   'curly-infix' read option, and the '#!curly-infix' and
>   '#!curly-infix-and-bracket-lists' reader directives.
>
> * doc/ref/api-options.texi (Runtime Options): Add 'curly-infix' to the
>   list of read options.
>
> * test-suite/Makefile.am: Add tests/srfi-105.test.
>
> * test-suite/tests/srfi-105.test: New file.

Besides, the patch is OK to apply for me, modulo the minor comments
below.

> +Guile also implements the following non-standard extension to SRFI-105:
> +if @code{curly-infix} is enabled but the @code{square-brackets} read
> +option is turned off, then lists within square brackets are read as
> +normal lists but with the special symbol @code{$bracket-list$} added to
> +the front.  To enable this combination of read options within a file,
> +use the reader directive @code{#!curly-infix-and-bracket-lists}.  For
> +example:

Do you think it would be possible, or even desirable, to be able to turn
off this extension?

>  scm_t_option scm_read_opts[] = {

Can you move that brace on the next line?

> @@ -98,6 +105,8 @@ struct t_read_opts {

Ditto.

> +      /* (len == 0) case is handled above */
> +      if (len == 1)
> +        /* Return directly to avoid re-annotating the element's source
> +           location with the position of the outer brace.  Also, it
> +           might not be possible to annotate the element. */
> +        return scm_car (ans);  /* {e} => e */
> +      else if (len == 2)
> +        ;  /* Leave the list unchanged: {e1 e2} => (e1 e2) */
> +      else if (len >= 3 && (len & 1))
> +        {
> +          SCM op = scm_cadr (ans);

Can you add a comment above this line describing what case this
corresponds to?  Like:

  This is a longer list; check whether it contains mixed infix
  operators, or if it’s an improper list.

> +scm_read_expression_1 (SCM port, scm_t_read_opts *opts)

What about calling it ‘read_inner_expression’, for instance?

> -#define READ_OPTIONS_NUM_BITS             14
> +#define READ_OPTIONS_NUM_BITS             16

I think I had missed that one.  Can you add a comment above to describe
it?

> +(define-module (test-srfi-105)
> +  #:use-module (test-suite lib)
> +  #:use-module (srfi srfi-1))
> +
> +#!curly-infix
> +
> +(with-test-prefix "curly-infix"
> +  (pass-if (equal? '{n <= 5}                '(<= n 5)))

This is testing both the #! syntax and the actual infix parsing.
Something like ‘with-read-options’ from reader.test could be used, but
perhaps that’s an unnecessary complication.

> +  ;;(pass-if (equal? '#1=f(#1#)               '#1=(f #1#)))

Not implemented yet?

> +  ;;(pass-if (equal? '#1={a + . #1#}          '($nfx$ . #1=(a + . #1#))))

Same?

This sounds like a great test suite.

However, could you add a few tests regarding source location tracking,
by reading from a string port?

Thanks!

Ludo’.