Re: [PATCH] Use a fluid for the list of the reader's "hash procedures"

[Ludovic Courtès]

Hi Andreas!

Andreas Rottmann <address@hidden> writes:

> address@hidden (Ludovic Courtès) writes:
>
>> Hi!
>>
>> Thanks for the patch.
>>
>> Andreas Rottmann <address@hidden> writes:
>>
>>> This is a prerequisite for the patch implementing SRFI-38, which I'll
>>> post next.
>>>
>>>
>>> From: Andreas Rottmann <address@hidden>
>>> Subject: Use a fluid for the list of the reader's "hash procedures"
>>>
>>> This allows customizing the reader behavior for a dynamic extent more 
>>> easily.
>>>
>>> * libguile/read.c (scm_read_hash_procedures): Renamed to
>>>   `scm_i_read_hash_procedures'.
>>>   (scm_i_read_hash_procedures_ref, scm_i_read_hash_procedures_set_x):
>>>   New (internal) accessor functions for the fluid.
>>>   (scm_read_hash_extend, scm_get_hash_procedure): Use these accessor
>>>   functions.
>>>   (scm_init_read): Create the fluid, named `%read-hash-procedures' instead 
>>> of 
>>>   the previous plain list `read-hash-procedures'.
>>
>> Hmm that’s an incompatible change.  ‘scm_read_hash_procedures’ and
>> ‘read-hash-procedures’ aren’t documented, but they’re public.  
>>
> The latter indeed was public -- I've added a compatibility shim,
> similiar to the one you suggested.

Great, applied!

> `scm_read_hash_procedures' was "static", so there should be no issue
> there.

Right.

> I kind-of assumed that undocumented functionality would not be held
> accountable to the normal deprecation rules.

Well, in some cases, we can’t ignore that undocumented functionality is
widely used; in other cases, we find no evidence that it was used.  So
undocumented functionality is really considered on a case-by-case basis.

> Now that we deprecate the functionality (and hence implicitly
> acknowledge that it might still be in use), I think it might make
> sense to either:
>
> - Make clear that `%read-hash-procedures' is "internal" -- the name kind
>   of implies that, but there are quite a few procedures that start with
>   `%' and are documented in the manual, so I cannot deduce what needs to
>   be done for a Scheme binding to be marked as internal, at least not
>   when it is defined in C and has to be accessible to the root module,
>   and hence is visible in the `(guile-user) module'
>
> - Document `%read-hash-procedures'.
>
> - Mark `%read-hash-procedures' as internal, but offer a documented way
>   to install a new fluid for a dynamic extent. E.g.:
>
> (define (with-read-hash-extensions extensions thunk)
>   (with-fluid* %read-hash-procedures (fluid-ref %read-hash-procedures)
>     (lambda ()
>       (for-each (lambda (extension)
>                    (read-hash-extend (car extension) (cdr extension)))
>                 extensions)
>       (thunk))))

Documenting ‘%read-hash-procedures’ is probably the easiest.  It exposes
implementation details, but OTOH, these details are unlikely to change
(there’s little incentive to change this.)

OTOOH (which one? :-)), ‘read-hash-extend’ exists precisely to hide
those details, which would warrant ‘with-read-hash-extensions’ as you
propose.

Thoughts?

>> The latter can probably be kept compatible like this:
>>
>>   (begin-deprecated
>>     (define %deprecated-read-hash-procedures
>>       (make-procedure-with-setter
>>         (lambda ()  (fluid-ref %read-hash-procedures))
>>         (lambda (v) (fluid-set! %read-hash-procedures v))))
>>
>>     (define-syntax read-hash-procedures
>>       (lambda (s)
>>         (syntax-case s ()
>>           (_ (%deprecated-read-hash-procedures))))))
>>
> Neat trick, I didn't know about `make-procedure-with-setter'.  My patch
> now contains equivalent functionality, but only relying on syntax-case,
> not `make-procedure-with-setter' -- it turned out you have to use
> `make-variable-transformer' or `identifier-syntax' if you want an
> identifier macro to work within a `set!' form.

Oh, nice.  I was unaware of the full power of ‘identifier-syntax’.

Thanks,
Ludo’.