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);





(On the C level, there is scm_i_procedure_arity.)

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

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

> +  scm_call_2(hook, full_filename, depth);

by



(using SCM_KEYWORD instead might be preferred).

Best regards,
Maxime Devos

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_3(hook, full_filename, scm_from_utf8_keyword("depth"), depth);

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature