* [PATCH] srfi-64: fix unused variable warnings @ 2021-04-01 6:11 Aleix Conchillo Flaqué 2021-04-01 11:37 ` Maxime Devos 0 siblings, 1 reply; 4+ messages in thread From: Aleix Conchillo Flaqué @ 2021-04-01 6:11 UTC (permalink / raw) To: guile-devel; +Cc: Aleix Conchillo Flaqué * module/srfi/srfi-64/testing.scm: remove unused name variable and use let instead of let*. Signed-off-by: Aleix Conchillo Flaqué <aconchillo@gmail.com> --- module/srfi/srfi-64/testing.scm | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/module/srfi/srfi-64/testing.scm b/module/srfi/srfi-64/testing.scm index 37792cd0f..d97797786 100644 --- a/module/srfi/srfi-64/testing.scm +++ b/module/srfi/srfi-64/testing.scm @@ -707,26 +707,24 @@ (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) () (((mac tname expr) line) (syntax - (let* ((r (test-runner-get)) - (name tname)) + (let ((r (test-runner-get))) (test-result-alist! r (cons (cons 'test-name tname) line)) (%test-comp1body r expr)))) (((mac expr) line) (syntax - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r line) (%test-comp1body r expr))))))) (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)))) (((mac expected expr) line comp) (syntax - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r line) (%test-comp2body r comp expected expr)))))) (define-syntax test-eqv @@ -740,13 +738,12 @@ (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) () (((mac tname expected expr error) line) (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 (%test-approximate= error) expected expr)))) (((mac expected expr error) line) (syntax - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r line) (%test-comp2body r (%test-approximate= error) expected expr)))))))) (else @@ -759,23 +756,21 @@ (define-syntax test-assert (syntax-rules () ((test-assert tname test-expression) - (let* ((r (test-runner-get)) - (name tname)) + (let ((r (test-runner-get))) (test-result-alist! r '((test-name . tname))) (%test-comp1body r test-expression))) ((test-assert test-expression) - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r '()) (%test-comp1body r test-expression))))) (define-syntax %test-comp2 (syntax-rules () ((%test-comp2 comp tname expected expr) - (let* ((r (test-runner-get)) - (name tname)) + (let ((r (test-runner-get))) (test-result-alist! r (list (cons 'test-name tname))) (%test-comp2body r comp expected expr))) ((%test-comp2 comp expected expr) - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r '()) (%test-comp2body r comp expected expr))))) (define-syntax test-equal @@ -895,18 +890,17 @@ (syntax-case (list x (list (syntax quote) (%test-source-line2 x))) () (((mac tname etype expr) line) (syntax - (let* ((r (test-runner-get)) - (name tname)) + (let ((r (test-runner-get))) (test-result-alist! r (cons (cons 'test-name tname) line)) (%test-error r etype expr)))) (((mac etype expr) line) (syntax - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r line) (%test-error r etype expr)))) (((mac expr) line) (syntax - (let* ((r (test-runner-get))) + (let ((r (test-runner-get))) (test-result-alist! r line) (%test-error r #t expr)))))))) (else -- 2.31.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] srfi-64: fix unused variable warnings 2021-04-01 6:11 [PATCH] srfi-64: fix unused variable warnings Aleix Conchillo Flaqué @ 2021-04-01 11:37 ` Maxime Devos 2021-04-02 6:13 ` Aleix Conchillo Flaqué [not found] ` <CA+XASoWMDakd2rAu96iZB-=giHLoer1gcM4dgu+wxg1MCZ9xDw@mail.gmail.com> 0 siblings, 2 replies; 4+ messages in thread From: Maxime Devos @ 2021-04-01 11:37 UTC (permalink / raw) To: Aleix Conchillo Flaqué, guile-devel [-- Attachment #1: Type: text/plain, Size: 2275 bytes --] On Wed, 2021-03-31 at 23:11 -0700, Aleix Conchillo Flaqué wrote: > * module/srfi/srfi-64/testing.scm: remove unused name variable and use > let instead of let*. > I don't think this is the correct approach with respect to side effects. For example, in: > (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)))) I would keep the let* (but reverse the binding order), but change 'tname' with 'name' in the call to 'test-result-alist!', such that 'test-X' macros behave somewhat more like procedure calls (except for installing exeption 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. 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)): (test-assert [test-name] expression) This evaluates the expression. The test passes if the result is true; if the result is false, a test failure is reported. The test also fails if an exception is raised, assuming the implementation has a way to catch exceptions. How the failure is reported depends on the test runner environment. The test-name is a string that names the test case. (Though the test-name is a string literal in the examples, it is an expression. ***It is evaluated only once.***) It is used when reporting errors, and also when skipping tests, as described below. It is an error to invoke test-assert if there is no current test runner. (My suggestion would be to also evaluate 'test-name' at least once, even if there is no test runner, which seems a bit stricter than SRFI-64 demands, but seems like a nice property to have and easy to achieve.) As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you change the patch message accordingly? Something like srfi-64: fix double evaluation of test-name. perhaps? Greetings, Maxime. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 260 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] srfi-64: fix unused variable warnings 2021-04-01 11:37 ` Maxime Devos @ 2021-04-02 6:13 ` Aleix Conchillo Flaqué [not found] ` <CA+XASoWMDakd2rAu96iZB-=giHLoer1gcM4dgu+wxg1MCZ9xDw@mail.gmail.com> 1 sibling, 0 replies; 4+ messages in thread From: Aleix Conchillo Flaqué @ 2021-04-02 6:13 UTC (permalink / raw) To: Maxime Devos; +Cc: guile-devel Hi Maxime, Thank you for your comments! On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <maximedevos@telenet.be> wrote: > For example, in: > > > (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)))) > > I would keep the let* (but reverse the binding order), but change 'tname' > with 'name' in the call to 'test-result-alist!', such that 'test-X' macros > behave somewhat more like procedure calls (except for installing exeption > 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. > 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"? > 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)): > > (test-assert [test-name] expression) > > This evaluates the expression. The test passes if the result is true; > if the result is false, a test failure is reported. The test also fails > if an exception is raised, assuming the implementation has a way to catch > exceptions. How the failure is reported depends on the test runner environment. > The test-name is a string that names the test case. (Though the test-name is > a string literal in the examples, it is an expression. ***It is evaluated only once.***) > It is used when reporting errors, and also when skipping tests, as described below. > It is an error to invoke test-assert if there is no current test runner. > > (My suggestion would be to also evaluate 'test-name' at least once, even if there > is no test runner, which seems a bit stricter than SRFI-64 demands, but seems like > a nice property to have and easy to achieve.) > Yes, this makes sense. Thanks again for pointing that out. > As this patch does not ‘merely’ fix a warnings, but fixes a bug, could you change > the patch message accordingly? Something like > > srfi-64: fix double evaluation of test-name. > > perhaps? > Sounds good to me. Best, Aleix ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <CA+XASoWMDakd2rAu96iZB-=giHLoer1gcM4dgu+wxg1MCZ9xDw@mail.gmail.com>]
* Re: [PATCH] srfi-64: fix unused variable warnings [not found] ` <CA+XASoWMDakd2rAu96iZB-=giHLoer1gcM4dgu+wxg1MCZ9xDw@mail.gmail.com> @ 2021-04-02 6:58 ` Maxime Devos 0 siblings, 0 replies; 4+ messages in thread From: Maxime Devos @ 2021-04-02 6:58 UTC (permalink / raw) To: Aleix Conchillo Flaqué; +Cc: guile-devel [-- Attachment #1: Type: text/plain, Size: 4507 bytes --] On Thu, 2021-04-01 at 23:12 -0700, Aleix Conchillo Flaqué wrote: > Hi Maxime, > > Thank you for your comments! > > On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <maximedevos@telenet.be> wrote: > > > For example, in: > > > > > (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)))) > > > > I would keep the let* (but reverse the binding order), but change 'tname' > > with 'name' in the call to 'test-result-alist!', such that 'test-X' macros > > behave somewhat more like procedure calls (except for installing exeption > > 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. > > > > 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 = (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 ‘call’ (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)): > > > > [...] > 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 ‘merely’ fix a warnings, but fixes a bug, could you change > > the patch message accordingly? Something like > > > > srfi-64: fix double evaluation of test-name. > > > > perhaps? > > 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. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 260 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-02 6:58 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-01 6:11 [PATCH] srfi-64: fix unused variable warnings Aleix Conchillo Flaqué 2021-04-01 11:37 ` Maxime Devos 2021-04-02 6:13 ` Aleix Conchillo Flaqué [not found] ` <CA+XASoWMDakd2rAu96iZB-=giHLoer1gcM4dgu+wxg1MCZ9xDw@mail.gmail.com> 2021-04-02 6:58 ` Maxime Devos
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).