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

[Maxim Cournoyer]

Hi Maxime,

Maxime Devos <maximedevos@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