[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] srfi-64: fix unused variable warnings

From: Aleix Conchillo Flaqué
Subject: Re: [PATCH] srfi-64: fix unused variable warnings
Date: Thu, 1 Apr 2021 23:13:25 -0700

Hi Maxime,

Thank you for your comments!

On Thu, Apr 1, 2021 at 4:37 AM Maxime Devos <> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]