Hi Maxime,
Maxime Devos <maximede...@telenet.be> writes:
> 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.
>>+ else {
>>+ /* Starting from 3.10, this function takes 1 required and 1 >optional
>>+ arguments. */
>>+ long len;
>>+
>>+ SCM_VALIDATE_LIST_COPYLEN (SCM_ARG1, args, len);
>>+ if (len < 1 || len > 2)
>>+ scm_error_num_args_subr (FUNC_NAME);
>>+
>>+ filename = SCM_CAR (args);
>>+ SCM_VALIDATE_STRING (SCM_ARG1, filename);
>>+
>>+ depth = len > 1 ? SCM_CADR (args) : scm_from_int(0);
>
> While I suppose this would work, you are using the rest argument to
> manually implement optional elements. I think it would be quite a bit
> simpler to just use the optional elements instead. (I.e., set opt=1
> instead of rst=1 and keep req=1 in SCM_DEFINE).
>
> Adding an extra argument is, however, a C API and ABI break, so I'd
> rename it to scm_primitive_load2 and let scm_primitive_load be a small
> wrapper for scm_primitive_load2.
>
> ... now I think about it and re-read the C comments, maybe the (lack
> of) C API/ABI break is why you are using rest arguments here? I think
> a new function is cleaner and less risky though -- I don't think you
> are supposed to do (primitive-load (list "a" 0)) in Scheme yet this
> code allows that.
>
> Likely the same applies to s_scm_primitive_load_path.
Yes, the extra complications on the C side are to preserve backward
compatibility.
>> * doc/guile-api.alist (%load-announce, %load-hook): Add DEPTH argument.
>
> Technically backwards-incompatible but I don't know any actual users
> of %load-hook etc. outside Guile itself.
%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.
--
Thanks,
Maxim
