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

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

Hi!

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

> ludo@gnu.org (Ludovic Courtès) writes:

>>>> Mark H Weaver <mhw@netris.org> skribis:
>>>>> +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.

[...]

>> Thus, I thought we’d logically have these 3 functions:
>> set_port_read_options, port_read_options, and applicable_read_options.
>
> Logically, I agree that this would be a nice interface.  The problem is
> really one of efficiency.  It's quite expensive to access the per-port
> read options directly, because it requires locking the port table mutex,
> doing a hash table lookup, and then an alist lookup.  That's not
> something I want to do more than once per call to 'read'.  (Even doing
> it once is slightly painful).

Understood.

> Efficiency is the main reason that I chose to compute all of the
> applicable read options and place them in OPTS at the start of 'read'.
> Efficiency is also the reason that I packed all of the read option
> overrides into a single integer.

Yes, that’s fine with me, as long as the visible interface maps as close
as possible to the underlying concepts.

>> Whether these are implemented in terms of bit fields is not the first
>> thing I want to see when I open read.c.
>>
>> Perhaps this is just a matter of presentation, but my impression was
>> that set_port_read_options and the various constants would force me to
>> think in terms of bit-twiddling more than in terms or read options.
>
> FWIW, all of the details of the bit-twiddling and the storage mechanism
> of per-port read options are confined to just two static functions:
> 'init_read_options' and 'set_per_port_read_option'.
>
> The rest of read.c needn't think about bit-twiddling at all.  The
> relevant interface for the rest of read.c is as follows:
>
> * Look up applicable read options in OPTS.
> * Set per-port read options by calling 'set_per_port_*'.

OK.  I’ll comment on the new version of your patches, thanks!

Ludo’.