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

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

ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@netris.org> skribis:
>> ludo@gnu.org (Ludovic Courtès) writes:
>>> 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?
>
> Yeah, we can think about it later.  The thing is, that API exists in
> read.c anyway, so I didn’t think it would be so much extra work.

APIs that we expose to the outside world need to be maintained
approximately forever, so we should expend a great deal of effort to
make sure they are future proof.  We don't have to worry so much about a
private interface that's accessible only within read.c.

> Now, I agree that the less we expose, the better.  ;-)

At least until we have the time to come up with a good interface.

>>> 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.
>>
>> 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.
>
> Well, I was more thinking in terms of the interface I’d like for the
> concepts at hand: we have per-ports and global settings, which we want
> to manipulate, and we want to know which ones are applicable at a given
> point.
>
> 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).

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.

> 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_*'.

So nothing else need think about the bit-twiddling.  That said, I agree
that it's unfortunate to see this bit-twiddling at the beginning of
read.c.  How about moving it to the end? :)

What do you think?

      Mark