From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Maxime Devos Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] srfi-64: fix unused variable warnings Date: Fri, 02 Apr 2021 08:58:40 +0200 Message-ID: References: <20210401061135.11914-1-aconchillo@gmail.com> <74a46c307dac2d9e2dbc4affdaa5c56a75fc2b1b.camel@telenet.be> Mime-Version: 1.0 Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-EEnuOQxKIstPDnSO2E1d" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="20689"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Evolution 3.34.2 Cc: guile-devel@gnu.org To: Aleix Conchillo =?ISO-8859-1?Q?Flaqu=E9?= Original-X-From: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Fri Apr 02 08:59:03 2021 Return-path: 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 ) id 1lSDlm-0005FU-VE for guile-devel@m.gmane-mx.org; Fri, 02 Apr 2021 08:59:03 +0200 Original-Received: from localhost ([::1]:47316 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lSDll-0005KU-Lm for guile-devel@m.gmane-mx.org; Fri, 02 Apr 2021 02:59:01 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:52790) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lSDlc-0005KO-4Z for guile-devel@gnu.org; Fri, 02 Apr 2021 02:58:52 -0400 Original-Received: from laurent.telenet-ops.be ([2a02:1800:110:4::f00:19]:40468) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lSDlZ-0004rV-H9 for guile-devel@gnu.org; Fri, 02 Apr 2021 02:58:51 -0400 Original-Received: from ptr-bvsjgyjmffd7q9timvx.18120a2.ip6.access.telenet.be ([IPv6:2a02:1811:8c09:9d00:aaf1:9810:a0b8:a55d]) by laurent.telenet-ops.be with bizsmtp id niyl2400U0mfAB401iymLM; Fri, 02 Apr 2021 08:58:46 +0200 In-Reply-To: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telenet.be; s=r21; t=1617346726; bh=x7rOfzV9aAaOksOHheEMd52jlKReVUSZ1KLTVSMmyT4=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=A5grmMMF9LDy2hyWdsrFaYbh4mI9TE6yUcB5n858Us5V75lLL9us0h4dWMUebHNoj WYkQ9cW/bFfLrJzirmv5Z7Uk7fmc1674xl+iFt+37POupvvIyEELndrBH8Y8XItTGC He6KOHMvmadl4YPxGPHf4Gi2dni/MqaYFjH0B60K5Yi6w3BRz1wkAslLJBRiSbA+Ry 5bbWF3YFn9CrZPUAHB/LuH7nLsUxk+7UQlJZti5K8kSGBc3ySWGkTPkivA3Tzt3PtM TtoXwFzrg8cmOl5k3FymZpzFMAobzREeY7+R1BL7G/ikTos4eMAHvut5MFkFMoGhzs SqGg9x2Zgn3NA== Received-SPF: pass client-ip=2a02:1800:110:4::f00:19; envelope-from=maximedevos@telenet.be; helo=laurent.telenet-ops.be X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 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, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane-mx.org@gnu.org Original-Sender: "guile-devel" Xref: news.gmane.io gmane.lisp.guile.devel:20724 Archived-At: --=-EEnuOQxKIstPDnSO2E1d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2021-04-01 at 23:12 -0700, Aleix Conchillo Flaqu=C3=A9 wrote: > Hi Maxime, >=20 > Thank you for your comments! >=20 > On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos wrot= e: >=20 > > For example, in: > >=20 > > > (define (%test-comp2 comp x) > > > (syntax-case (list x (list (syntax quote) (%test-source-line2 x)= ) comp) () > > > (((mac tname expected expr) line comp) > > > (syntax > > > - (let* ((r (test-runner-get)) > > > - (name tname)) > > > + (let ((r (test-runner-get))) > > > (test-result-alist! r (cons (cons 'test-name tname) line)) > > > (%test-comp2body r comp expected expr)))) > >=20 > > I would keep the let* (but reverse the binding order), but change 'tnam= e' > > with 'name' in the call to 'test-result-alist!', such that 'test-X' mac= ros > > behave somewhat more like procedure calls (except for installing exepti= on > > handlers and having access to the s-expression of the code that will be= run, > > of course). It's largely a matter of taste, though. > >=20 >=20 > I've done this change. One thing I don't understand is the "reverse > the binding order", I've done it as suggested but is this change the > one you refer to as "matter of taste"? Yes, that's the change I was referring to. As to why: a procedural equivalent of 'test-assert would look more or less like ;; (possibly more arguments are required) (define* (test-assert* name thunk expression) ;; THUNK: when called, return something that will be ;; used as true/false. ;; EXPRESSION: S-expression representing the body ;; of THUNK (let ((r (test-runner-get))) ;; evaluate (thunk) here within some exception ;; handlers and use r ...)) (Similar equivalents to test-equal, test-eq ... can be written as well.) Suppose '(test-assert* NAME (lambda () EXP) 'EXP) is evaluated. Then first NAME is evaluated, which can have side-effects. The lambda expression and (quote EXP) are evaluated as well, but no side-effects are possible here (aside for allocating some memory for the thunk, which can lead to a 'out-of-memory exception, but that's usually simply ignored). Only after the arguments are evaluated will '(test-runner-get) be evaluated. However, for the original test-assert macro, the evaluation order is different. From a REPL: > (use-modules (srfi srfi-26)) > ,expand (test-assert NAME EXP) ;; output manually cleaned up $6 =3D (let* ((r (test-runner-get)) (name NAME)) more code ....) It should be clear that here 'NAME is evaluated *after* '(test-runner-get) is evaluated, unlike for the 'test-assert* procedure. That said, SRFI-64 does not require NAME to be evaluated even if trying to get the test runner fails for some reason, I don't think anyone ever changes the test runner from within a =E2=80=98call=E2=80=99 (not really a call as test-assert is a macro) to test-assert, and in practice NAME is a constant, so in practice it doesn't really matter in what order things are evaluated. Also see next comment: > > In any case, it is good that 'tname' is now evaluated only once, as per > > SRFI-64 (notice ***It is evaluated only once.*** (markup mine)): > >=20 > > [...] > Yes, this makes sense. Thanks again for pointing that out. This is done correctly in the new patch (and the old patch IIRC). Also, by reversing the binding order from - (let* ((r (test-runner-get)) - (name tname)) to + (let* ((name tname) + (r (test-runner-get))) the expression tname is also evaluated *at least* once, thus TNAME is evaluated *exactly* once, which seems like a nice property to have, though this is a bit stricter than SRFI-64 demands IIUC. Also, the formatting seems to have gone wrong. Shouldn't this be + (let* ((name tname) + (r (test-runner-get))) ? If in Emacs, I recommend scheme-mode, in which case pressing tab on the second line would produce the desired formatting. Alternatively, select a region of text and presss tab. > > As this patch does not =E2=80=98merely=E2=80=99 fix a warnings, but fix= es a bug, could you change > > the patch message accordingly? Something like > >=20 > > srfi-64: fix double evaluation of test-name. > >=20 > > perhaps? > >=20 The revised commit message looks good to me. Greetings, Maxime. p.s: I'm not a guile maintainer so you will have to wait on someone else to actually merge this. --=-EEnuOQxKIstPDnSO2E1d Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYGbAoBccbWF4aW1lZGV2 b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lzuAQCY74iwh0MPH4mHxb4ZcPzOa/BE QZTmB6UjFGFX+hb0KgEAliQM3AdBKIR+IbinI2TZSWcONr4nvUPCay/QGC+KcgM= =uOr+ -----END PGP SIGNATURE----- --=-EEnuOQxKIstPDnSO2E1d--