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

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

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

> From 255aaaf0f474d45bd67d6b3b102b2806a8f0db97 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Tue, 23 Oct 2012 00:50:42 -0400
> Subject: [PATCH 7/9] Implement per-port read options.
>
> * libguile/read.c (scm_t_read_opts): Update comment to mention the
>   per-port read options.
>
>   (sym_read_option_overrides): New symbol.
>
>   (set_per_port_read_option): New internal static function.

The patch add this static function, but leaves it unused.  And also,
there are no tests.

So, what about exposing a ‘set-port-read-options!’ procedure, and then
using it to write tests?

>   (init_read_options): Add new 'port' parameter, and consult the
>   per-port read option overrides when initializing the 'scm_t_read_opts'
>   struct.
>
>   (scm_read): Pass 'port' parameter to init_read_options.
>
> * doc/ref/api-evaluation.texi (Scheme Read): Mention the existence of
>   (currently unused) per-port reader options.

OK.

> +Note that Guile also includes a preliminary mechanism for overriding
> +read options on a per-port basis, but it is currently unused and there
> +is no way to access or set these per-port read options.

I think you can remove this paragraph because it becomes largely invalid
with the next few patches.

> +/*
> + * Per-port read option overrides.
> + *
> + * We store per-port read option overrides in the
> + * '%read-option-overrides%' key of the port's alist, which is stored in
> + * 'scm_i_port_weak_hash'.  The value stored in the alist is a single
> + * integer that contains a two-bit field for each read option.
> + *
> + * If a bit field contains OVERRIDE_DEFAULT (3), that indicates that the
> + * corresponding read option has not been overridden for this port, so
> + * the global read option should be used.  Otherwise, the bit field
> + * contains the value of the read option.  For boolean read options that
> + * have been overridden, the other possible values are 0 or 1.  If the
> + * 'keyword_style' read option is overridden, its possible values are
> + * taken from the enum of the 'scm_t_read_opts' struct.
> + */

Tricky semantics, but I guess there’s no other way.

> +SCM_SYMBOL (sym_read_option_overrides, "%read-option-overrides%");

Maybe ‘read-option-overrides’ is enough since it’s an internal alist
anyway.

> +/* Offsets of bit fields for each per-port override */
> +#define OVERRIDE_SHIFT_COPY_SOURCE_P          0
> +#define OVERRIDE_SHIFT_RECORD_POSITIONS_P     2
> +#define OVERRIDE_SHIFT_CASE_INSENSITIVE_P     4
> +#define OVERRIDE_SHIFT_KEYWORD_STYLE          6
> +#define OVERRIDE_SHIFT_R6RS_ESCAPES_P         8
> +#define OVERRIDE_SHIFT_SQUARE_BRACKETS_P     10
> +#define OVERRIDE_SHIFT_HUNGRY_EOL_ESCAPES_P  12
> +#define OVERRIDES_SHIFT_END                  14
> +
> +#define OVERRIDES_ALL_DEFAULTS  ((1UL << OVERRIDES_SHIFT_END) - 1)
> +#define OVERRIDES_MAX_VALUE     OVERRIDES_ALL_DEFAULTS
> +
> +#define OVERRIDE_MASK     3
> +#define OVERRIDE_DEFAULT  3

What about s/OVERRIDE_SHIFT/READ_OPTION/?  And:

> +set_per_port_read_option (SCM port, int shift, int value)

Also change ‘shift’ to ‘option’, and ‘int value’ to something like
‘enum t_option_state value’, where:

  enum t_option_state
  {
    OPTION_INHERITED,    /* global option setting inherited */
    OPTION_DISABLED,
    OPTION_ENABLED
  };

the goal being to hide as much of the bit-twiddling as possible.

What about also adding:

  static int per_port_read_option (SCM port, int option);
  static int applicable_read_option (SCM port, int option);

(Maybe it comes next?)

> +/* Initialize the internal read options structure from the global and
> +   per-port read options. */
> +static void
> +init_read_options (SCM port, scm_t_read_opts *opts)

Rather along the lines of “Initialize OPTS based on PORT’s read options
and the global read options.”

> +#define RESOLVE_BOOLEAN_OPTION(NAME, name)                       \
> +  do {                                                           \
> +    x = OVERRIDE_MASK & (overrides >> OVERRIDE_SHIFT_ ## NAME);  \
> +    if (x == OVERRIDE_DEFAULT)                                   \
> +      x = !!SCM_ ## NAME;                                        \
> +    opts->name = x;                                              \
> +  } while (0)

Braces misplaced.

Thanks,
Ludo’.