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

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3068 bytes --]



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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]