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

[Mark H Weaver <mhw@netris.org>]

Hi Ludovic,

Thanks for your review.  I have pushed the first six patches, with all
of your suggestions applied except that I didn't make 'scm_t_read_opts'
const, because it needs to be mutated when encountering a reader
directive, as you later acknowledged on IRC.

ludo@gnu.org (Ludovic Courtès) writes:
> 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.

That's because you asked me to split the patch into pieces :-P

It only causes a warning, and the immediately following patch makes use
of this function and tests the functionality.

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

That's a lot of extra work.  It means designing, implementing, and
documenting a new non-trivial API that we'll have to maintain forever.
I'd rather not do that work now.  I'm quite overloaded and have more
important things to do.

Can the API be added later, by someone who is motivated to do that work?

[... skipped several more comments that I agree with ...]

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

Right now, this single function can be used for all the options (both
the boolean options and the keyword style option).  If I change it as
you suggest, then I would have to split it into two nearly-identical
functions, and it wouldn't hide _any_ bit-twiddling.  Apart from
duplicating the code, the only changes would be to rename
OVERRIDE_DEFAULT to OPTION_INHERITED, and to make the non-inherit case
more complex by changing a simple assignment (of the 2-bit bit-field
into scm_t_read_opts) into a switch statement to convert these new enum
values into a value appropriate for scm_t_read_opts.

Is this added complexity really necessary?  This is all internal logic
that's confined to a few static functions in read.c.

> What about also adding:
>
>   static int per_port_read_option (SCM port, int option);
>   static int applicable_read_option (SCM port, int option);

Who would use these functions?

>> +/* 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.”

Okay.

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

Okay.

    Thanks,
      Mark