unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
@ 2024-02-13 21:21 Konstantin Kharlamov
  2024-02-14  1:01 ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Konstantin Kharlamov @ 2024-02-13 21:21 UTC (permalink / raw)
  To: 69108

I've been writing an answer for a question on emacs.stackexchange¹ and to avoid
nested `if` and `let` clauses I used a `if-let*`, and result of one of the checks I
assigned to a `_` variable, because the variable would be left unused, it's only the
check being non-nil that mattered.

But when byte-compiled that triggered a:

    test.el:6:9: Warning: variable ‘_’ not left unused

…which is untrue, because it is unused.

The problem is present in both `if-let` and `if-let*`

# Steps to reproduce

1. Create test.el with the following code:

    ;;; -*- lexical-binding: t -*-
    (if-let*
        ((_ nil))
        (print "then clause")
      (print "else clause"))

2. M-x byte-compile test.el

## Expected

It byte-compiles with no warnings

## Actual

It compiles with a warning:

    test.el:3:7: Warning: variable ‘_’ not left unused

# Additional information

Emacs version: commit d4d5830f8a0 built two weeks ago from master.

1: https://emacs.stackexchange.com/questions/80351/delete-prettify-symbol





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-13 21:21 bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let Konstantin Kharlamov
@ 2024-02-14  1:01 ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-17  0:28   ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-14  1:01 UTC (permalink / raw)
  To: Konstantin Kharlamov; +Cc: 69108

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> I've been writing an answer for a question on emacs.stackexchange¹ and
> to avoid nested `if` and `let` clauses I used a `if-let*`, and result
> of one of the checks I assigned to a `_` variable, because the
> variable would be left unused, it's only the check being non-nil that
> mattered.
>
> But when byte-compiled that triggered a:
>
>     test.el:6:9: Warning: variable ‘_’ not left unused
>
> …which is untrue, because it is unused.

I also find this annoying.

Currently the variable is actually used (implicitly, in the expansion),
so it's not an error in the compiler.

But the warning is not really helpful (code works as intended), and

  (_ TEST-EXPR)

is maybe even easier to read or more intuitive than the official

  (TEST-EXPR)

syntax.

Would be a one liner to make both cases generate the same expansion.
Are there any votes against this?


Michael.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-14  1:01 ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-17  0:28   ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-17  8:04     ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-17  0:28 UTC (permalink / raw)
  To: Konstantin Kharlamov; +Cc: 69108

[-- Attachment #1: Type: text/plain, Size: 109 bytes --]

I <michael_heerdegen@web.de> write:

> Would be a one liner to make both cases generate the same expansion.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-warn-about-_-not-left-unused-in-if-let-and-ali.patch --]
[-- Type: text/x-diff, Size: 3024 bytes --]

From 906355a716864c87aa0ea112ac890ec9f59d0089 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Fri, 16 Feb 2024 22:07:18 +0100
Subject: [PATCH] Don't warn about _ not left unused in if-let and alike

Fix Bug#69108: The macro expansions did not leave a variable _ unused;
this triggered a compiler warning.

* lisp/subr.el (internal--build-binding): Handle (_ FORM) separately.
(if-let, and-let*): Tweak doc.
---
 lisp/subr.el | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index c317d558e24..4f22f0c6b3f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2575,12 +2575,12 @@ delay-mode-hooks
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."
   (setq binding
-        (cond
-         ((symbolp binding)
+        (pcase binding
+         ((pred symbolp)
           (list binding binding))
-         ((null (cdr binding))
-          (list (make-symbol "s") (car binding)))
-         (t binding)))
+         ((or `(,test) `(_ ,test))
+          (list (make-symbol "s") test))
+         (_ binding)))
   (when (> (length binding) 2)
     (signal 'error
             (cons "`let' bindings can have only one value-form" binding)))
@@ -2620,7 +2620,7 @@ when-let*
 (defmacro and-let* (varlist &rest body)
   "Bind variables according to VARLIST and conditionally evaluate BODY.
 Like `when-let*', except if BODY is empty and all the bindings
-are non-nil, then the result is non-nil."
+are non-nil, then the result is the value of the last binding."
   (declare (indent 1) (debug if-let*))
   (let (res)
     (if varlist
@@ -2631,9 +2631,9 @@ and-let*

 (defmacro if-let (spec then &rest else)
   "Bind variables according to SPEC and evaluate THEN or ELSE.
-Evaluate each binding in turn, as in `let*', stopping if a
-binding value is nil.  If all are non-nil return the value of
-THEN, otherwise the last form in ELSE.
+Evaluate each binding in turn, as in `let*', stopping if a binding value
+is nil.  If all are non-nil return the value of THEN, otherwise the
+value of the last ELSE form or nil if there are none.

 Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
 SYMBOL to the value of VALUEFORM.  An element can additionally be
@@ -2642,9 +2642,9 @@ if-let
 interest.  It can also be of the form SYMBOL, then the binding of
 SYMBOL is checked for nil.

-As a special case, interprets a SPEC of the form \(SYMBOL SOMETHING)
-like \((SYMBOL SOMETHING)).  This exists for backward compatibility
-with an old syntax that accepted only one binding."
+As a special case that exists for backward compatibility only, a
+complete SPEC of the form \(SYMBOL SOMETHING) is interpreted like
+\((SYMBOL SOMETHING))."
   (declare (indent 2)
            (debug ([&or (symbolp form)  ; must be first, Bug#48489
                         (&rest [&or symbolp (symbolp form) (form)])]
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 11 bytes --]



Michael.

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17  0:28   ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-17  8:04     ` Eli Zaretskii
  2024-02-17  9:20       ` Konstantin Kharlamov
  2024-02-17 22:02       ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2024-02-17  8:04 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 69108, Hi-Angel

> Cc: 69108@debbugs.gnu.org
> Date: Sat, 17 Feb 2024 01:28:55 +0100
> From:  Michael Heerdegen via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -2575,12 +2575,12 @@ delay-mode-hooks
>  (defun internal--build-binding (binding prev-var)
>    "Check and build a single BINDING with PREV-VAR."
>    (setq binding
> -        (cond
> -         ((symbolp binding)
> +        (pcase binding
> +         ((pred symbolp)
>            (list binding binding))
> -         ((null (cdr binding))
> -          (list (make-symbol "s") (car binding)))
> -         (t binding)))
> +         ((or `(,test) `(_ ,test))
> +          (list (make-symbol "s") test))
> +         (_ binding)))

Thanks, but can we please leave this as 'cond', instead of converting
it to a 'pcase'?  It doesn't seem to be justified here, and even less
so since you need to rewrite all the existing conditions.

>  (defmacro if-let (spec then &rest else)
>    "Bind variables according to SPEC and evaluate THEN or ELSE.
> -Evaluate each binding in turn, as in `let*', stopping if a
> -binding value is nil.  If all are non-nil return the value of
> -THEN, otherwise the last form in ELSE.
> +Evaluate each binding in turn, as in `let*', stopping if a binding value
> +is nil.  If all are non-nil return the value of THEN, otherwise the
> +value of the last ELSE form or nil if there are none.
> 
>  Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
>  SYMBOL to the value of VALUEFORM.  An element can additionally be
> @@ -2642,9 +2642,9 @@ if-let
>  interest.  It can also be of the form SYMBOL, then the binding of
>  SYMBOL is checked for nil.
> 
> -As a special case, interprets a SPEC of the form \(SYMBOL SOMETHING)
> -like \((SYMBOL SOMETHING)).  This exists for backward compatibility
> -with an old syntax that accepted only one binding."
> +As a special case that exists for backward compatibility only, a
> +complete SPEC of the form \(SYMBOL SOMETHING) is interpreted like
> +\((SYMBOL SOMETHING))."
>    (declare (indent 2)
>             (debug ([&or (symbolp form)  ; must be first, Bug#48489
>                          (&rest [&or symbolp (symbolp form) (form)])]

This hunk seems to be unrelated?  And it is not necessarily for the
better, IMO, at least not all of it (replaces active tense with
passive, refills text that doesn't need refilling, and other minor
issues, like the confusing use of construct state in "last ELSE
form").





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17  8:04     ` Eli Zaretskii
@ 2024-02-17  9:20       ` Konstantin Kharlamov
  2024-02-17 11:45         ` Ihor Radchenko
  2024-02-17 21:46         ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-17 22:02       ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 2 replies; 14+ messages in thread
From: Konstantin Kharlamov @ 2024-02-17  9:20 UTC (permalink / raw)
  To: Eli Zaretskii, Michael Heerdegen; +Cc: 69108

On Sat, 2024-02-17 at 10:04 +0200, Eli Zaretskii wrote:
> > Cc: 69108@debbugs.gnu.org
> > Date: Sat, 17 Feb 2024 01:28:55 +0100
> > From:  Michael Heerdegen via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> > @@ -2575,12 +2575,12 @@ delay-mode-hooks
> >  (defun internal--build-binding (binding prev-var)
> >    "Check and build a single BINDING with PREV-VAR."
> >    (setq binding
> > -        (cond
> > -         ((symbolp binding)
> > +        (pcase binding
> > +         ((pred symbolp)
> >            (list binding binding))
> > -         ((null (cdr binding))
> > -          (list (make-symbol "s") (car binding)))
> > -         (t binding)))
> > +         ((or `(,test) `(_ ,test))
> > +          (list (make-symbol "s") test))
> > +         (_ binding)))
> 
> Thanks, but can we please leave this as 'cond', instead of converting
> it to a 'pcase'?  It doesn't seem to be justified here, and even less
> so since you need to rewrite all the existing conditions.

Just a side note, from my experience pcase is very slow¹, so if a
function supposed to be called often, I presume it's better to avoid
`pcase`. Although, Idk how it compares to `cond`. But judging from the
fact `cond` is implemented in C, it is likely faster.

1:
https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17 11:45         ` Ihor Radchenko
@ 2024-02-17 10:09           ` Konstantin Kharlamov
  2024-02-19 12:37             ` Ihor Radchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Konstantin Kharlamov @ 2024-02-17 10:09 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Michael Heerdegen, Eli Zaretskii, 69108

On Sat, 2024-02-17 at 11:45 +0000, Ihor Radchenko wrote:
> Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:
>
> > Just a side note, from my experience pcase is very slow¹, so if a
> > function supposed to be called often, I presume it's better to
> > avoid
> > `pcase`. Although, Idk how it compares to `cond`. But judging from
> > the
> > fact `cond` is implemented in C, it is likely faster.
> >
> > 1:
> > https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09
>
> I very much doubt the assertion of that commit.
> AFAIK, pcase expands to a similar consp check. If may be slow only
> when
> you macro-expand it during run time, not byte-compiling the code
> during
> benchmark. I recommend `benchmark-run-compiled' for testing. Or even
> to
> use native-compilation.

Sure, I just re-tested with `benchmark-run-compiled` and the result is similar.

Here's what I do:

1. In color-identifers repo `git checkout bc566bcdbd79f230b35eafd2b6c4f8428402ec09`
2. Open a `emacs ./color-identifiers-mode.el`
3. Now, I don't know for sure if `benchmark-run-compiled` compiles child functions or
   not. But just to be on the safe side I merged the two functions, so:

   1. Inside function `color-identifiers:elisp-get-declarations` remove the call
      `(color-identifiers:elisp-declarations-in-sexp sexp result)`
   2. Copy the body of `color-identifiers:elisp-declarations-in-sexp` starting with
      the first `(let)` call and paste it in place where we removed the call.
4. Evaluate `color-identifiers:elisp-get-declarations`
5. Open Emacs's `simple.el`
6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`
   Results for 3 times are (this is another laptop, so numbers are a bit different):

    (2.0673167670000003 54 1.316455474999998)
    (2.0684198769999997 54 1.3043319699999998)
    (2.079789413 54 1.3183175779999985)

7. Undo code changes and call `git checkout HEAD^` to test code prior to the commit
8. Repeat 3 and 4
9. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`.
   Results for 3 times are:

    (5.194122174 135 3.313309744999998)
    (5.130884611 135 3.2485326989999948)
    (5.155663089 134 3.2561076369999995)





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17  9:20       ` Konstantin Kharlamov
@ 2024-02-17 11:45         ` Ihor Radchenko
  2024-02-17 10:09           ` Konstantin Kharlamov
  2024-02-17 21:46         ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 14+ messages in thread
From: Ihor Radchenko @ 2024-02-17 11:45 UTC (permalink / raw)
  To: Konstantin Kharlamov; +Cc: Michael Heerdegen, Eli Zaretskii, 69108

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> Just a side note, from my experience pcase is very slow¹, so if a
> function supposed to be called often, I presume it's better to avoid
> `pcase`. Although, Idk how it compares to `cond`. But judging from the
> fact `cond` is implemented in C, it is likely faster.
>
> 1:
> https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09

I very much doubt the assertion of that commit.
AFAIK, pcase expands to a similar consp check. If may be slow only when
you macro-expand it during run time, not byte-compiling the code during
benchmark. I recommend `benchmark-run-compiled' for testing. Or even to
use native-compilation.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17  9:20       ` Konstantin Kharlamov
  2024-02-17 11:45         ` Ihor Radchenko
@ 2024-02-17 21:46         ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 0 replies; 14+ messages in thread
From: Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-17 21:46 UTC (permalink / raw)
  To: Konstantin Kharlamov; +Cc: Eli Zaretskii, 69108

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> Just a side note, from my experience pcase is very slow¹, so if a
> function supposed to be called often,

The function I changed is only called when generating the expansions of
`if-let' etc, so this is not really an issue here.

> I presume it's better to avoid `pcase`. Although, Idk how it compares
> to `cond`. But judging from the fact `cond` is implemented in C, it is
> likely faster.

?... pcase also expands to things that are implemented in C, this is a
too simple benchmark.  If you want to discuss this, maybe in a different
thread, as it is not relevant here.

Michael.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17  8:04     ` Eli Zaretskii
  2024-02-17  9:20       ` Konstantin Kharlamov
@ 2024-02-17 22:02       ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-18  6:53         ` Eli Zaretskii
  1 sibling, 1 reply; 14+ messages in thread
From: Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-17 22:02 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Hi-Angel, 69108

Eli Zaretskii <eliz@gnu.org> writes:

> > Cc: 69108@debbugs.gnu.org
> > Date: Sat, 17 Feb 2024 01:28:55 +0100
> > From:  Michael Heerdegen via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> > @@ -2575,12 +2575,12 @@ delay-mode-hooks
> >  (defun internal--build-binding (binding prev-var)
> >    "Check and build a single BINDING with PREV-VAR."
> >    (setq binding
> > -        (cond
> > -         ((symbolp binding)
> > +        (pcase binding
> > +         ((pred symbolp)
> >            (list binding binding))
> > -         ((null (cdr binding))
> > -          (list (make-symbol "s") (car binding)))
> > -         (t binding)))
> > +         ((or `(,test) `(_ ,test))
> > +          (list (make-symbol "s") test))
> > +         (_ binding)))
>
> Thanks, but can we please leave this as 'cond', instead of converting
> it to a 'pcase'?  It doesn't seem to be justified here, and even less
> so since you need to rewrite all the existing conditions.

Oh no.

If I don't rewrite this with `pcase', we would either artificially split
this case:

  ((or `(,test) `(_ ,test))
   (list (make-symbol "s") test))

into two separate `cond' branches, or we had to merge them into a one branch
like this:

  ((or (null (cdr binding))
       (eq '_ (car binding)))
   (list (make-symbol "s")
         (if (null (cdr binding))
             (car binding)
           (cadr binding))))

repeating a test.  Is this what you prefer?

We could also move the test for _ to the beginning, destroying the logic
of the code.  All of those alternatives seems worse wrt readability.

Please to everyone: let's avoid a new discussion about `pcase'.  Please,
not again.

> > [My doc tweaks]
> This hunk seems to be unrelated?

Yes, I can make it a separate commit it drop it entirely if you prefer.

> And it is not necessarily for the better, IMO, at least not all of it
> (replaces active tense with passive, refills text that doesn't need
> refilling, and other minor issues,

I can try to improve that of course.

> like the confusing use of construct state in "last ELSE form").

Dunno what a "construct state" is.  The doc missed to tell what `if-let'
returns when optional ELSE forms are omitted (which is allowed, and then
there is no last ELSE form return value), so I tried to add that.  Did I
mess up the grammar?

Michael.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17 22:02       ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-18  6:53         ` Eli Zaretskii
  2024-02-25  1:54           ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2024-02-18  6:53 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: Hi-Angel, 69108

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 69108@debbugs.gnu.org,  Hi-Angel@yandex.ru
> Date: Sat, 17 Feb 2024 23:02:07 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but can we please leave this as 'cond', instead of converting
> > it to a 'pcase'?  It doesn't seem to be justified here, and even less
> > so since you need to rewrite all the existing conditions.
> 
> Oh no.
> 
> If I don't rewrite this with `pcase', we would either artificially split
> this case:
> 
>   ((or `(,test) `(_ ,test))
>    (list (make-symbol "s") test))
> 
> into two separate `cond' branches, or we had to merge them into a one branch
> like this:
> 
>   ((or (null (cdr binding))
>        (eq '_ (car binding)))
>    (list (make-symbol "s")
>          (if (null (cdr binding))
>              (car binding)
>            (cadr binding))))
> 
> repeating a test.  Is this what you prefer?

Yes, I think so.  And you could perhaps avoid repetition of
(cdr binding) by saving the result of (null (cdr binding)) in
a local variable.

Or did I misunderstand the issues?

> Please to everyone: let's avoid a new discussion about `pcase'.  Please,
> not again.

It isn't a discussion.  I'm asking you (and everyone else) to avoid
using pcase where a simple cond will do, certainly when changing code
that already uses cond for most of the conditions.  That will both
decrease the code churn, and thus minimize the probability of
inadvertent mistakes, and make the code easier to read for some.

> > > [My doc tweaks]
> > This hunk seems to be unrelated?
> 
> Yes, I can make it a separate commit it drop it entirely if you prefer.

If it's unrelated, then yes, I'd prefer to separate it.

> > And it is not necessarily for the better, IMO, at least not all of it
> > (replaces active tense with passive, refills text that doesn't need
> > refilling, and other minor issues,
> 
> I can try to improve that of course.
> 
> > like the confusing use of construct state in "last ELSE form").
> 
> Dunno what a "construct state" is.

  https://en.wikipedia.org/wiki/Construct_state

It has to do with juxtaposition of several nouns to express genitive.
In this case I meant the replacement of "last form in ELSE" with "the
last ELSE form", which is more confusing, because it isn't clear
whether "last" refers to "ELSE" or to "form".

> The doc missed to tell what `if-let' returns when optional ELSE
> forms are omitted (which is allowed, and then there is no last ELSE
> form return value), so I tried to add that.  Did I mess up the
> grammar?

The grammar might be okay, but "last form in ELSE" is still better,
and your rewording lacks crucial punctuation which could have helped
interpreting the text correctly.  For example, there should be a comma
before "or nil if there are none".





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-17 10:09           ` Konstantin Kharlamov
@ 2024-02-19 12:37             ` Ihor Radchenko
  2024-02-19 13:44               ` Konstantin Kharlamov
  0 siblings, 1 reply; 14+ messages in thread
From: Ihor Radchenko @ 2024-02-19 12:37 UTC (permalink / raw)
  To: Konstantin Kharlamov; +Cc: Michael Heerdegen, Eli Zaretskii, 69108

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> 6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`

This is not enough. You also need to run
(byte-compile #'color-identifiers:elisp-get-declarations)

With this, I am getting

(0.014252469 0 0.0)
(0.014183416999999999 0 0.0)
with pcase

and

(0.014351118 0 0.0)
(0.014329416 0 0.0)
with cond

No measurable difference.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-19 12:37             ` Ihor Radchenko
@ 2024-02-19 13:44               ` Konstantin Kharlamov
  0 siblings, 0 replies; 14+ messages in thread
From: Konstantin Kharlamov @ 2024-02-19 13:44 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Michael Heerdegen, Eli Zaretskii, 69108

You were right after all 😊 I confirm your findings, thank you for
explanation!

On Mon, 2024-02-19 at 12:37 +0000, Ihor Radchenko wrote:
> Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:
> 
> > 6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-
> > get-declarations))`
> 
> This is not enough. You also need to run
> (byte-compile #'color-identifiers:elisp-get-declarations)
> 
> With this, I am getting
> 
> (0.014252469 0 0.0)
> (0.014183416999999999 0 0.0)
> with pcase
> 
> and
> 
> (0.014351118 0 0.0)
> (0.014329416 0 0.0)
> with cond
> 
> No measurable difference.
> 






^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-18  6:53         ` Eli Zaretskii
@ 2024-02-25  1:54           ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-25  7:42             ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-25  1:54 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Hi-Angel, 69108

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> > [...] repeating a test.  Is this what you prefer?
>
> Yes, I think so.  And you could perhaps avoid repetition of
> (cdr binding) by saving the result of (null (cdr binding)) in
> a local variable.

I went with a separate `cond' branch added instead, I think this is even
simpler.  Ok?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-warn-about-_-not-left-unused-in-if-let-and-ali.patch --]
[-- Type: text/x-diff, Size: 986 bytes --]

From 83d42089a77bc0fa4745f708ca73b5e7cddd1829 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Fri, 16 Feb 2024 22:07:18 +0100
Subject: [PATCH 1/2] Don't warn about _ not left unused in if-let and alike

The macro expansions did not leave a variable _ unused; this triggered
an irritating compiler warning (bug#69108).

* lisp/subr.el (internal--build-binding): Handle bindings of the form
(_ EXPR) separately.
---
 lisp/subr.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/subr.el b/lisp/subr.el
index c317d558e24..afbe6845d7a 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2580,6 +2580,8 @@ internal--build-binding
           (list binding binding))
          ((null (cdr binding))
           (list (make-symbol "s") (car binding)))
+         ((eq '_ (car binding))
+          (list (make-symbol "s") (cadr binding)))
          (t binding)))
   (when (> (length binding) 2)
     (signal 'error
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 174 bytes --]



> > {My unsuccessful doc tweaks}
>
> [...] I'd prefer to separate it.

Done.  Feel free to tune it to your likes.  Or send me an "ok", then
I'll just commit this version.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-WIP-lisp-subr.el-if-let-and-let-Tweak-doc.patch --]
[-- Type: text/x-diff, Size: 1330 bytes --]

From 117a82505c3fe9ec1148cb2a7b870cb8e2eb2b6d Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Sun, 18 Feb 2024 02:27:56 +0100
Subject: [PATCH 2/2] WIP: ; * lisp/subr.el (if-let, and-let*): Tweak doc

---
 lisp/subr.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index afbe6845d7a..62600ff49bf 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2622,7 +2622,7 @@ when-let*
 (defmacro and-let* (varlist &rest body)
   "Bind variables according to VARLIST and conditionally evaluate BODY.
 Like `when-let*', except if BODY is empty and all the bindings
-are non-nil, then the result is non-nil."
+are non-nil, then the result is the value of the last binding."
   (declare (indent 1) (debug if-let*))
   (let (res)
     (if varlist
@@ -2635,7 +2635,8 @@ if-let
   "Bind variables according to SPEC and evaluate THEN or ELSE.
 Evaluate each binding in turn, as in `let*', stopping if a
 binding value is nil.  If all are non-nil return the value of
-THEN, otherwise the last form in ELSE.
+THEN, otherwise the value of the last form in ELSE, or nil if
+there are none.

 Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
 SYMBOL to the value of VALUEFORM.  An element can additionally be
--
2.39.2


[-- Attachment #5: Type: text/plain, Size: 146 bytes --]



>   https://en.wikipedia.org/wiki/Construct_state

Thanks.  Did not expect that my change could be interpreted involving
genitive...


Michael.

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
  2024-02-25  1:54           ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-25  7:42             ` Eli Zaretskii
  0 siblings, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2024-02-25  7:42 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 69108-done, Hi-Angel

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 69108@debbugs.gnu.org,  Hi-Angel@yandex.ru
> Date: Sun, 25 Feb 2024 02:54:04 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > [...] repeating a test.  Is this what you prefer?
> >
> > Yes, I think so.  And you could perhaps avoid repetition of
> > (cdr binding) by saving the result of (null (cdr binding)) in
> > a local variable.
> 
> I went with a separate `cond' branch added instead, I think this is even
> simpler.  Ok?

Yes, thanks.

> > > {My unsuccessful doc tweaks}
> >
> > [...] I'd prefer to separate it.
> 
> Done.  Feel free to tune it to your likes.  Or send me an "ok", then
> I'll just commit this version.

I installed them both, thanks.

Closing the bug.





^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-02-25  7:42 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-13 21:21 bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let Konstantin Kharlamov
2024-02-14  1:01 ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-17  0:28   ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-17  8:04     ` Eli Zaretskii
2024-02-17  9:20       ` Konstantin Kharlamov
2024-02-17 11:45         ` Ihor Radchenko
2024-02-17 10:09           ` Konstantin Kharlamov
2024-02-19 12:37             ` Ihor Radchenko
2024-02-19 13:44               ` Konstantin Kharlamov
2024-02-17 21:46         ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-17 22:02       ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-18  6:53         ` Eli Zaretskii
2024-02-25  1:54           ` Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-25  7:42             ` Eli Zaretskii

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

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).