Mark H Weaver <m...@netris.org> skribis:
> From 255aaaf0f474d45bd67d6b3b102b2806a8f0db97 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <m...@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’.
