Hi Andreas! Andreas Rottmann <a.rottm...@gmx.at> writes:
> l...@gnu.org (Ludovic Courtès) writes: > >> Hi! >> >> Thanks for the patch. >> >> Andreas Rottmann <a.rottm...@gmx.at> writes: >> >>> This is a prerequisite for the patch implementing SRFI-38, which I'll >>> post next. >>> >>> >>> From: Andreas Rottmann <a.rottm...@gmx.at> >>> 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’.
