l...@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <m...@netris.org> skribis:
>> l...@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 <m...@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
