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

[Andreas Rottmann]

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.  `scm_read_hash_procedures' was
"static", so there should be no issue there.

I kind-of assumed that undocumented functionality would not be held
accountable to the normal deprecation rules.  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))))

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

read-hash-fluid.diff
Description: Text Data

Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>