From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Maxim Cournoyer <maxim.cournoyer@gmail.com> Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH v4 4/4] load: Display modules depth in output when using %load-verbosely. Date: Sun, 22 Oct 2023 00:14:41 -0400 Message-ID: <878r7vcmz2.fsf@gmail.com> References: <20230925142945.14153-1-maxim.cournoyer@gmail.com> <20230925142945.14153-5-maxim.cournoyer@gmail.com> <6f5ace52-5ade-cfe2-bbf6-22562db70206@telenet.be> <87jzs53suy.fsf@gmail.com> <f8f71dc7-3270-5282-32b7-91a28800b43a@telenet.be> <87v8bn5fk1.fsf@gmail.com> <c03b3c3b-c4d1-1e29-a278-51848ed1a967@telenet.be> <87r0m1evgt.fsf@gmail.com> <a61ddf09-3647-b402-3fb6-0a86aefa735a@telenet.be> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23760"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: guile-devel@gnu.org To: Maxime Devos <maximedevos@telenet.be> Original-X-From: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Sun Oct 22 06:15:18 2023 Return-path: <guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org> Envelope-to: guile-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org>) id 1quPry-0005zT-Kf for guile-devel@m.gmane-mx.org; Sun, 22 Oct 2023 06:15:18 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from <guile-devel-bounces@gnu.org>) id 1quPrb-0002IU-SR; Sun, 22 Oct 2023 00:14:55 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from <maxim.cournoyer@gmail.com>) id 1quPrX-0002IF-Ge for guile-devel@gnu.org; Sun, 22 Oct 2023 00:14:51 -0400 Original-Received: from mail-qv1-xf2e.google.com ([2607:f8b0:4864:20::f2e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from <maxim.cournoyer@gmail.com>) id 1quPrR-0004hB-JG for guile-devel@gnu.org; Sun, 22 Oct 2023 00:14:51 -0400 Original-Received: by mail-qv1-xf2e.google.com with SMTP id 6a1803df08f44-65af7d102b3so15538846d6.1 for <guile-devel@gnu.org>; Sat, 21 Oct 2023 21:14:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697948084; x=1698552884; darn=gnu.org; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to; bh=CQPDyjLqBgpmPZvoKwYQoVf1Z1s8DAcgbW30Qv347Gg=; b=F5R2FclchFLbAfj94TEGUp3tRP2FV5NTKPxXdUs9mh21kfquHV0WcZKWVmK+rF9498 Gs39GxNXIBbCZqBpV6OMFVTTJDU7d/d2KdIBYzykPGnPr0Sz+SX+wi4eJ1siil516e98 7mwjhRl5mnAgMExrTNj8NPgWlXtFQlK+eJVReFavZv084Co+nWsuAGPPF3asQ1VVpbUy p0EOLmuqJXPO68kyYnHZ9tcpNdPwGkj0QvaW80bJn3R2txVl/SVWBoZThCahfdEF90FF PIAUyBCw2yutC/6mWUm/xKVRodrBnRy/XIx4lCAvrECyMNngY5uxGbaUuJp5PpSuyV08 Lkkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697948084; x=1698552884; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=CQPDyjLqBgpmPZvoKwYQoVf1Z1s8DAcgbW30Qv347Gg=; b=DdWmZGP/1hEGli1beg8p597LJVHhE4Q2wYa0btCzBthx46wEMdxbERm5HqA+JMhpyJ hqBiir5pWewi33gFtDDQss8IH2noIFeOmagZckl1xD9+vOnBw/NgC7n+iWEcRq/Ko51a jcXPj0JPlukHqPzAhJ+PQRKMTgvUCm3hFyUpQVOwENtnQ4COgpyXu6LpaHNGGzRHDEcV Rz/ZVoFcSqJj+YBclXXuuFQa17dU80d61WDFWC0+ZSSsHDFatTS01Rj/Juw93k+YD3G4 CbH4Ztrur/GQ5hDdTmISp3nDsairZbppRPCwlI4HfjDC0YC0rpHpN2zYF4zTFAMNfK3G zjcA== X-Gm-Message-State: AOJu0Yy1h+mRIk0MpkI/pmgI5w/fFsTuP591fXVcFL64zcaj8YFPbDSq c2/taf7VstV1l1bx6llqOhlHAsw8sYg= X-Google-Smtp-Source: AGHT+IEMJtuqDg51AZpYWqn0ecjpJ45h0GAMafqKzeJWkvhxm1haf2+WUu8u51eehHuYnqsg/sYrMQ== X-Received: by 2002:a05:6214:628:b0:66d:253c:9a80 with SMTP id a8-20020a056214062800b0066d253c9a80mr7286296qvx.54.1697948084207; Sat, 21 Oct 2023 21:14:44 -0700 (PDT) Original-Received: from hurd (dsl-205-236-230-191.b2b2c.ca. [205.236.230.191]) by smtp.gmail.com with ESMTPSA id y18-20020a0cead2000000b0066d15e2b73fsm1878304qvp.145.2023.10.21.21.14.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 21 Oct 2023 21:14:43 -0700 (PDT) In-Reply-To: <a61ddf09-3647-b402-3fb6-0a86aefa735a@telenet.be> (Maxime Devos's message of "Wed, 11 Oct 2023 14:37:14 +0200") Received-SPF: pass client-ip=2607:f8b0:4864:20::f2e; envelope-from=maxim.cournoyer@gmail.com; helo=mail-qv1-xf2e.google.com X-Spam_score_int: 6 X-Spam_score: 0.6 X-Spam_bar: / X-Spam_report: (0.6 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, GUARANTEED_100_PERCENT=2.699, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" <guile-devel.gnu.org> List-Unsubscribe: <https://lists.gnu.org/mailman/options/guile-devel>, <mailto:guile-devel-request@gnu.org?subject=unsubscribe> List-Archive: <https://lists.gnu.org/archive/html/guile-devel> List-Post: <mailto:guile-devel@gnu.org> List-Help: <mailto:guile-devel-request@gnu.org?subject=help> List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guile-devel>, <mailto:guile-devel-request@gnu.org?subject=subscribe> Errors-To: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.lisp.guile.devel:22032 Archived-At: <http://permalink.gmane.org/gmane.lisp.guile.devel/22032> --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Maxime, Maxime Devos <maximedevos@telenet.be> writes: [...] >>> 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 =E2=80=98no arity information -> assume new >>> interface'.) >>> >>> (On the C level, there is scm_i_procedure_arity.) >> I see what you mean about potential nested wrong-number-of-args being >> raised by the hook procedure itself, but I'm not sure how that can be >> improved. I had tried introspecting the arity of a procedure and it >> didn't work on the C side, at least using 'scm_procedure_minimum_arity' >> (which is implemented in terms of scm_i_procedure_arity). From my >> #guile IRC logs: >> --8<---------------cut here---------------start------------->8--- >> 2023-09-08 21:13:50 apteryx interesting, arity =3D >> scm_procedure_minimum_arity (hook); doesn't work in load.c >> 2023-09-08 21:14:03 apteryx it produces arity=3D(0 0 #t) >> --8<---------------cut here---------------end--------------->8--- >> Also, what do you mean by 'not 100% guaranteed' ? I think catching >> too >> many errors here is a better trade than catching none. > > I think the opposite -- if the load hook really wants to catch its own > wrong-number-of-args, it can just do that by inserting an appropriate > 'catch', whereas the load hook cannot remove the overly wide > implicitly catch of primitive_load. > Implicitly catching the errors is also an API violation, as it is not > documented that primitive-load, load-in-vicinity, etc., implicitly > suppress errors. OK, I agree! I tried using scm_program_minimum_arity in libguile/load.c like so: --8<---------------cut here---------------start------------->8--- modified libguile/load.c @@ -52,6 +52,7 @@ #include "loader.h" #include "modules.h" #include "pairs.h" +#include "procprop.h" #include "procs.h" #include "read.h" #include "srfi-13.h" @@ -80,21 +81,26 @@ static SCM *scm_loc_load_hook; /* The current reader (a fluid). */ static SCM the_reader =3D SCM_BOOL_F; =20 -struct hook_args_data { - SCM filename; - SCM depth; -}; +/* Predicate to check whether HOOK is of the new type, accepting extra + keyword arguments and a rest argument. */ +static SCM new_hook_p (SCM hook) { + SCM arity; + uint nreqs; /* number of required arguments */ + uint nopts; /* number of optional arguments */ + SCM restp; /* accepts rest argument? */ =20 -static SCM call_hook_2_body(void *data) { - struct hook_args_data *args_data =3D data; - scm_call_2(*scm_loc_load_hook, args_data->filename, args_data->depth); - return SCM_BOOL_T; -} + arity =3D scm_procedure_minimum_arity(hook); =20 -static SCM call_hook_1_handler(void *data, SCM key, SCM args ) { - struct hook_args_data *args_data =3D data; - scm_call_1(*scm_loc_load_hook, args_data->filename); + /* In case the hook has no arity information, simply assumes it uses + the newer interface. */ + if (scm_is_eq(SCM_BOOL_F, arity)) return SCM_BOOL_T; + + nreqs =3D scm_to_uint(scm_car(arity)); + nopts =3D scm_to_uint(scm_cadr(arity)); + restp =3D scm_caddr(arity); + + return scm_from_bool(!restp && (nreqs + nopts) <=3D 1); } =20 /* Helper to call %load-hook with the correct number of arguments. */ @@ -102,15 +108,11 @@ static void call_hook (SCM hook, SCM filename, SCM de= pth) { if (scm_is_false (hook)) return; =20 - struct hook_args_data args_data; - args_data.filename =3D filename; - args_data.depth =3D depth; - - /* 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); + if (new_hook_p(hook)) { + scm_call_3(hook, filename, scm_from_utf8_keyword("depth"), depth); + } else { + scm_call_1(hook, filename); + } } =20 SCM_DEFINE (scm_primitive_load, "primitive-load", 0, 0, 1, --8<---------------cut here---------------end--------------->8--- Unfortunately, it fails to build with an error that hints that perhaps using scm_procedure_minimum_arity that early is not supported? --8<---------------cut here---------------start------------->8--- Making all in module make[2]: Entering directory '/home/maxim/src/guile/module' make[2]: Nothing to be done for 'all'. make[2]: Leaving directory '/home/maxim/src/guile/module' Making all in stage0 make[2]: Entering directory '/home/maxim/src/guile/stage0' make[2]: Nothing to be done for 'all'. make[2]: Leaving directory '/home/maxim/src/guile/stage0' Making all in stage1 make[2]: Entering directory '/home/maxim/src/guile/stage1' BOOTSTRAP(stage1) GUILEC ice-9/boot-9.go ;;; note: source file /home/maxim/src/guile/module/ice-9/boot-9.scm ;;; newer than compiled /home/maxim/src/guile/stage1/ice-9/boot-9.go ;;; found fresh compiled file at /home/maxim/src/guile/stage0/ice-9/boot-9.= go guile: uncaught exception: In procedure private-lookup: Module named (system vm program) does not exist Cannot exit gracefully when init is in progress; aborting. make[2]: *** [Makefile:2512: ice-9/boot-9.go] Aborted make[2]: Leaving directory '/home/maxim/src/guile/stage1' make[1]: *** [Makefile:2205: all-recursive] Error 1 make[1]: Leaving directory '/home/maxim/src/guile' make: *** [Makefile:2090 : all] Erreur 2 --8<---------------cut here---------------end--------------->8--- See the attached patch file below if you'd like to experiment with it. --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-on-top-of-v4-arity-check-wip.patch >From a0463d1b2aee68587e4183df1be3a16ba445ad3c Mon Sep 17 00:00:00 2001 From: Maxim Cournoyer <maxim.cournoyer@gmail.com> Date: Sat, 21 Oct 2023 23:12:08 -0400 Subject: [PATCH] on top of v4 arity check wip --- doc/ref/api-evaluation.texi | 6 ++--- libguile/load.c | 44 +++++++++++++++++++------------------ module/ice-9/boot-9.scm | 5 +++-- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/doc/ref/api-evaluation.texi b/doc/ref/api-evaluation.texi index 2cccf0481..6cc32ca17 100644 --- a/doc/ref/api-evaluation.texi +++ b/doc/ref/api-evaluation.texi @@ -872,8 +872,8 @@ top-level environment. @var{filename} must either be a full pathname or be a pathname relative to the current directory. If the variable @code{%load-hook} is defined, it should be bound to a procedure that will be called before any code is loaded. See the documentation for -@code{%load-hook} later in this section. An optional second argument, -@var{depth}, can be specified to track the depth at which modules are +@code{%load-hook} later in this section. An optional keyword argument, +@var{#:depth}, can be specified to track the depth at which modules are loaded. For compatibility with Guile 3.9 and earlier, the C function takes only @@ -921,7 +921,7 @@ The default @code{%load-hook} is bound to a procedure that does something like: @example -(define (%load-hook file depth) +(define* (%load-hook file #:key (depth 0) #:allow-other-keys) (when %load-verbosely (let* ((pad-count (- 3 (string-length (number->string depth)))) (pad (if (> pad-count 0) diff --git a/libguile/load.c b/libguile/load.c index 094b6d985..3af7d8844 100644 --- a/libguile/load.c +++ b/libguile/load.c @@ -52,6 +52,7 @@ #include "loader.h" #include "modules.h" #include "pairs.h" +#include "procprop.h" #include "procs.h" #include "read.h" #include "srfi-13.h" @@ -80,21 +81,26 @@ static SCM *scm_loc_load_hook; /* The current reader (a fluid). */ static SCM the_reader = SCM_BOOL_F; -struct hook_args_data { - SCM filename; - SCM depth; -}; +/* Predicate to check whether HOOK is of the new type, accepting extra + keyword arguments and a rest argument. */ +static SCM new_hook_p (SCM hook) { + SCM arity; + uint nreqs; /* number of required arguments */ + uint nopts; /* number of optional arguments */ + SCM restp; /* accepts rest argument? */ -static SCM call_hook_2_body(void *data) { - struct hook_args_data *args_data = data; - scm_call_2(*scm_loc_load_hook, args_data->filename, args_data->depth); - return SCM_BOOL_T; -} + arity = scm_procedure_minimum_arity(hook); -static SCM call_hook_1_handler(void *data, SCM key, SCM args ) { - struct hook_args_data *args_data = data; - scm_call_1(*scm_loc_load_hook, args_data->filename); + /* In case the hook has no arity information, simply assumes it uses + the newer interface. */ + if (scm_is_eq(SCM_BOOL_F, arity)) return SCM_BOOL_T; + + nreqs = scm_to_uint(scm_car(arity)); + nopts = scm_to_uint(scm_cadr(arity)); + restp = scm_caddr(arity); + + return scm_from_bool(!restp && (nreqs + nopts) <= 1); } /* Helper to call %load-hook with the correct number of arguments. */ @@ -102,15 +108,11 @@ static void call_hook (SCM hook, SCM filename, SCM depth) { if (scm_is_false (hook)) return; - struct hook_args_data args_data; - args_data.filename = filename; - args_data.depth = depth; - - /* 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); + if (new_hook_p(hook)) { + scm_call_3(hook, filename, scm_from_utf8_keyword("depth"), depth); + } else { + scm_call_1(hook, filename); + } } SCM_DEFINE (scm_primitive_load, "primitive-load", 0, 0, 1, diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm index a1177e172..ad9ace8c8 100644 --- a/module/ice-9/boot-9.scm +++ b/module/ice-9/boot-9.scm @@ -2236,7 +2236,7 @@ name extensions listed in %load-extensions." (define %load-verbosely #f) (define (assert-load-verbosity v) (set! %load-verbosely v)) -(define (%load-announce file depth) +(define* (%load-announce file #:key (depth 0) #:allow-other-keys) (when %load-verbosely (let* ((pad-count (- 3 (string-length (number->string depth)))) (pad (if (> pad-count 0) @@ -4416,7 +4416,8 @@ when none is available, reading FILE-NAME with READER." (if compiled (begin (if %load-hook - (%load-hook abs-file-name (%current-module-load-depth))) + (%load-hook abs-file-name + #:depth (%current-module-load-depth))) (compiled)) (start-stack 'load-stack (primitive-load abs-file-name))))) base-commit: 8441d8ff5671db690eb239cfea4dcfdee6d6dcdb prerequisite-patch-id: 73b2c2e4cdba4082690935d77d8b9df1175d82c8 prerequisite-patch-id: a4a96623fa9d08a3032734e30e01808c1f646956 prerequisite-patch-id: f983f023aedefef1c1ecfb07e99555a57ac2b62a prerequisite-patch-id: 4745e61c60672254d05ee5936b0750bb0a904a47 -- 2.41.0 --=-=-= Content-Type: text/plain > (Also, you forgot doing the arity checks in ice-9/boot-9.scm; there > are users of %load-hook outside boot-9.scm.) Ugh. Thanks for spotting that. [...] >>>>> 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). >> Thanks for the concrete example. I think I was thinking in terms of >> scm_primitive_load, which is the one carrying the extra information >> provided to the %load-hook (e.g. the depth) during its recursive calls. >> Since we're stuck with positional arguments for scm_primitive_load for >> backward compatibility, I see little value having a more flexible arity >> for the %load-hook (they will have to evolve together if they do, as I >> see it). > > We are not stuck with the positional arguments. There is no backwards > incompatibility on the Scheme side (besides the initial 'add rest > arguments' or 'add keyword arguments' thing'), and on the C side you > could simply define scm_primitive_load2, late scm_primitive_load3, > etc., or the hack that you are using currently. [...] I'm convinced here again, thanks for all the details and thoughtful examples! I've made the change, and given the above error encountered trying to go fancy with the arity, I've just thrown the towel on that and accepted a backward incompatible change for user hooks arity, that I've documented in NEWS. A v5 revision of this series will be sent soon, and you'll be CC'd on it. -- Thanks, Maxim --=-=-=--