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

[Ludovic Courtès]

Hi Mark!

Mark H Weaver <address@hidden> skribis:

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

Yes, thanks.

> address@hidden (Ludovic Courtès) writes:
>> Mark H Weaver <address@hidden> skribis:
>>> From 255aaaf0f474d45bd67d6b3b102b2806a8f0db97 Mon Sep 17 00:00:00 2001
>>> From: Mark H Weaver <address@hidden>
>>> 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?

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.

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

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

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.

WDYT?

Ludo’.