Re: [PATCH v4 4/4] load: Display modules depth in output when using %load-verbosely.

[Maxime Devos]

Op 04-10-2023 om 03:42 schreef Maxim Cournoyer:
> > > %load-hook is carefully crafted as to accept both one or two arguments
> > > (for backward compatibility).  I didn't pay attention to %load-announce
> > > and I'm surprised it's a public API, as its sole function seems to be
> > > the default %load-hook value.
> >
> > The default value might be, but it's supposed to be overwritable, and
> > according to the old API it's a one-argument procedure, so if there is
> > an old application/library setting it to something accepting only a
> > single argument, there is your API break.
>
> %load-announce is replaced whole via something like:
>
>    set! %load-hook my-proc
>
> placed early in your program.  Whether my-proc is a 1 or 2 arguments
> procedure, it works with the proposed change (it's been tested).

I now see:

> +  /* For compatibility with older load hooks procedures, fall-back to
> +     calling it with a single argument if calling it with two fails. */
> +  scm_internal_catch (scm_from_latin1_symbol ("wrong-number-of-args"),
> +                      call_hook_2_body, &args_data,
> +                      call_hook_1_handler, &args_data);

But that doesn't work properly, as it catches too much -- the 
'wrong-numer-of-args' might be caused by something inside the handler 
instead of an incorrect-arity invocation of the handler itself.

Something like 'program-arity' would be more accurate, albeit not 100% 
guaranteed.  But for backwards compatibility it might be good enough.
(Caveat: I don't know if uncompiled procedures have arity information, 
though perhaps you could go ‘no arity information -> assume new interface'.)

(On the C level, there is scm_i_procedure_arity.)

> > To prevent future API breaks, I propose documenting turning %load-hook
> > into a keyword argument procedure with #:allow-other-keys, as
> > something like:
> >
> >    (lambda* (file-name #:key (depth the-default) #:allow-other-keys)
> >      ...)
> >
> > and in the documentation mention that more keywords may be added in
> > the future (and hence #:allow-other-keys).
> >
> > I think it's quite plausible that there will be more such arguments in
> > the future!
>
> That sounds like a good idea, helas as I understand, with the current
> solution, everything needs to be kept as fixed positional arguments so
> we can make sense of them on the C side (which accepts a list of 1 or
> more items, expected to be given in order).  So unless you have other
> ideas that would also ensure backward-compatibility concern on the C
> side, I'd leave it as is for now

The C side doesn't expect anything -- the C side only _calls_ the load 
hook, it doesn't implement the load hook, as far as I can tell.

More concretely, as I understand it, all you need to do on the C-side is 
replacing

> +  scm_call_2(hook, full_filename, depth);

by

> +  scm_call_3(hook, full_filename,  scm_from_utf8_keyword("depth"), 
depth);

(using SCM_KEYWORD instead might be preferred).

Best regards,
Maxime Devos

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature