[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 4/4] load: Display modules depth in output when using %loa
From: |
Maxim Cournoyer |
Subject: |
Re: [PATCH v4 4/4] load: Display modules depth in output when using %load-verbosely. |
Date: |
Tue, 03 Oct 2023 21:42:22 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Hello!
Maxime Devos <maximedevos@telenet.be> writes:
> Op 02-10-2023 om 18:13 schreef Maxim Cournoyer:
>>> Something I didn't notice previously:
>>>
>>> Op 25-09-2023 om 16:29 schreef Maxim Cournoyer:
>>>> + if (scm_is_string (args)) {
>>>> + /* C code written for 3.9 and earlier expects this function to
>>>> + take a single argument (the file name). */
>>>> + filename = args;
>>>> + depth = scm_from_int(0);
>>>> + }
>>> IIUC, args is always a list and never a string. However, it might be
>>> a list of one element (being a string) or two elements.
>> Surprisingly perhaps, this works, I guess because a string is an array.
>> See the 2009 commit 31ab99de563027fe2bceb60bbd712407fcaf868e which
>> exploits the same feature.
>
> While apparently it works, that does not look like a feature to me but
> rather an accident, though I don't know what hypothetical feature you
> are referring to.
I meant feature here as behavior.
> Also, sure strings are arrays, but you don't seem
> to be using that anywhere, so I'm not following.
That was just an uneducated guess about how the underlying C procedure
translated from the macros. You can safely forget about it.
>> %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).
> 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.
--
Thanks,
Maxim