unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
@ 2022-08-31 12:02 Augusto Stoffel
  2022-08-31 12:47 ` Philip Kaludercic
  0 siblings, 1 reply; 10+ messages in thread
From: Augusto Stoffel @ 2022-08-31 12:02 UTC (permalink / raw)
  To: 57502

This buffer-match-p condition does the expected job:

     (buffer-match-p '(or "\\*" (derived-mode . special-mode))
                     (current-buffer))

But this presumably equivalent one gives a “(wrong-type-argument listp
special-mode)” error:

     (buffer-match-p '(or (and "\\*")
                          (derived-mode . special-mode))
                     (current-buffer))





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-08-31 12:02 bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p Augusto Stoffel
@ 2022-08-31 12:47 ` Philip Kaludercic
  2022-08-31 12:50   ` Philip Kaludercic
  0 siblings, 1 reply; 10+ messages in thread
From: Philip Kaludercic @ 2022-08-31 12:47 UTC (permalink / raw)
  To: Augusto Stoffel; +Cc: 57502

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

Augusto Stoffel <arstoffel@gmail.com> writes:

> This buffer-match-p condition does the expected job:
>
>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>                      (current-buffer))
>
> But this presumably equivalent one gives a “(wrong-type-argument listp
> special-mode)” error:
>
>      (buffer-match-p '(or (and "\\*")
>                           (derived-mode . special-mode))
>                      (current-buffer))

It seems to me that the issue is related to the `and' being wrapped by
an `or', specifically because of a typo in the handling of `and':


[-- Attachment #2: Type: text/plain, Size: 637 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 2ffc594997..0350c16ccf 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7014,8 +7014,8 @@ buffer-match-p
                       (funcall match (cdr condition)))
                      ((eq (car-safe condition) 'and)
                       (catch 'fail
-                        (dolist (c (cdr conditions))
-                          (unless (funcall match c)
+                        (dolist (c (cdr condition))
+                          (unless (funcall match (list c))
                             (throw 'fail nil)))
                         t)))
                 (throw 'match t)))))))

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


As you have pointed out to me privately, it might make sense to rewrite
the case distinction using pcase, to avoid simple mistakes like these.

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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-08-31 12:47 ` Philip Kaludercic
@ 2022-08-31 12:50   ` Philip Kaludercic
  2022-08-31 16:22     ` Augusto Stoffel
  2022-09-03 11:04     ` Philip Kaludercic
  0 siblings, 2 replies; 10+ messages in thread
From: Philip Kaludercic @ 2022-08-31 12:50 UTC (permalink / raw)
  To: Augusto Stoffel; +Cc: 57502

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

Philip Kaludercic <philipk@posteo.net> writes:

> Augusto Stoffel <arstoffel@gmail.com> writes:
>
>> This buffer-match-p condition does the expected job:
>>
>>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>>                      (current-buffer))
>>
>> But this presumably equivalent one gives a “(wrong-type-argument listp
>> special-mode)” error:
>>
>>      (buffer-match-p '(or (and "\\*")
>>                           (derived-mode . special-mode))
>>                      (current-buffer))
>
> It seems to me that the issue is related to the `and' being wrapped by
> an `or', specifically because of a typo in the handling of `and':
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 2ffc594997..0350c16ccf 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -7014,8 +7014,8 @@ buffer-match-p
>                        (funcall match (cdr condition)))
>                       ((eq (car-safe condition) 'and)
>                        (catch 'fail
> -                        (dolist (c (cdr conditions))
> -                          (unless (funcall match c)
> +                        (dolist (c (cdr condition))
> +                          (unless (funcall match (list c))
>                              (throw 'fail nil)))
>                          t)))
>                  (throw 'match t)))))))
>
>
> As you have pointed out to me privately, it might make sense to rewrite
> the case distinction using pcase, to avoid simple mistakes like these.

That might look something like this:


[-- Attachment #2: Type: text/plain, Size: 2849 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 2ffc594997..db1dc25044 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6992,32 +6992,32 @@ buffer-match-p
         (lambda (conditions)
           (catch 'match
             (dolist (condition conditions)
-              (when (cond
-                     ((eq condition t))
-                     ((stringp condition)
-                      (string-match-p condition (buffer-name buffer)))
-                     ((functionp condition)
-                      (if (eq 1 (cdr (func-arity condition)))
-                          (funcall condition buffer)
-                        (funcall condition buffer arg)))
-                     ((eq (car-safe condition) 'major-mode)
-                      (eq
-                       (buffer-local-value 'major-mode buffer)
-                       (cdr condition)))
-                     ((eq (car-safe condition) 'derived-mode)
-                      (provided-mode-derived-p
-                       (buffer-local-value 'major-mode buffer)
-                       (cdr condition)))
-                     ((eq (car-safe condition) 'not)
-                      (not (funcall match (cdr condition))))
-                     ((eq (car-safe condition) 'or)
-                      (funcall match (cdr condition)))
-                     ((eq (car-safe condition) 'and)
-                      (catch 'fail
-                        (dolist (c (cdr conditions))
-                          (unless (funcall match c)
-                            (throw 'fail nil)))
-                        t)))
+              (when (pcase condition
+                      ('t t)
+                      ((pred stringp)
+                       (string-match-p condition (buffer-name buffer)))
+                      ((pred functionp)
+                       (if (eq 1 (cdr (func-arity condition)))
+                           (funcall condition buffer)
+                         (funcall condition buffer arg)))
+                      (`(major-mode . ,mode)
+                       (eq
+                        (buffer-local-value 'major-mode buffer)
+                        mode))
+                      (`(derived-mode . ,mode)
+                       (provided-mode-derived-p
+                        (buffer-local-value 'major-mode buffer)
+                        mode))
+                      (`(not . cond)
+                       (not (funcall match cond)))
+                      (`(or . ,args)
+                       (funcall match args))
+                      (`(and . ,args)
+                       (catch 'fail
+                         (dolist (c args)
+                           (unless (funcall match (list c))
+                             (throw 'fail nil)))
+                         t)))
                 (throw 'match t)))))))
     (funcall match (list condition))))
 

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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-08-31 12:50   ` Philip Kaludercic
@ 2022-08-31 16:22     ` Augusto Stoffel
  2022-08-31 16:30       ` Philip Kaludercic
  2022-09-03 11:04     ` Philip Kaludercic
  1 sibling, 1 reply; 10+ messages in thread
From: Augusto Stoffel @ 2022-08-31 16:22 UTC (permalink / raw)
  To: Philip Kaludercic; +Cc: 57502

On Wed, 31 Aug 2022 at 12:50, Philip Kaludercic <philipk@posteo.net> wrote:

> That might look something like this:
>
[...]
> +                      (`(derived-mode . ,mode)
> +                       (provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))

On a tangent, wouldn't it be nice to allow a list of modes?  That is,

> +                      (`(derived-mode . ,modes)
> +                       (apply #'provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        modes))

(with some extra care to keep backwards compatibility).





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-08-31 16:22     ` Augusto Stoffel
@ 2022-08-31 16:30       ` Philip Kaludercic
  0 siblings, 0 replies; 10+ messages in thread
From: Philip Kaludercic @ 2022-08-31 16:30 UTC (permalink / raw)
  To: Augusto Stoffel; +Cc: 57502

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Wed, 31 Aug 2022 at 12:50, Philip Kaludercic <philipk@posteo.net> wrote:
>
>> That might look something like this:
>>
> [...]
>> +                      (`(derived-mode . ,mode)
>> +                       (provided-mode-derived-p
>> +                        (buffer-local-value 'major-mode buffer)
>> +                        mode))
>
> On a tangent, wouldn't it be nice to allow a list of modes?  That is,
>
>> +                      (`(derived-mode . ,modes)
>> +                       (apply #'provided-mode-derived-p
>> +                        (buffer-local-value 'major-mode buffer)
>> +                        modes))
>
> (with some extra care to keep backwards compatibility).

Intuitively I feel as though this could be more problematic/confusing
than convenient.  If this is done, then it should be supported in every
case, so that

      (and (derived-mode foo-mode)
           (major-mode . bar-mode))

(I know this is a stupid example) doesn't make someone want to try

      (and (derived-mode foo-mode)
           (major-mode bar-mode))

and be confused why it fails.





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-08-31 12:50   ` Philip Kaludercic
  2022-08-31 16:22     ` Augusto Stoffel
@ 2022-09-03 11:04     ` Philip Kaludercic
  2022-09-03 11:10       ` Eli Zaretskii
  1 sibling, 1 reply; 10+ messages in thread
From: Philip Kaludercic @ 2022-09-03 11:04 UTC (permalink / raw)
  To: Augusto Stoffel; +Cc: 57502

Philip Kaludercic <philipk@posteo.net> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Augusto Stoffel <arstoffel@gmail.com> writes:
>>
>>> This buffer-match-p condition does the expected job:
>>>
>>>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>>>                      (current-buffer))
>>>
>>> But this presumably equivalent one gives a “(wrong-type-argument listp
>>> special-mode)” error:
>>>
>>>      (buffer-match-p '(or (and "\\*")
>>>                           (derived-mode . special-mode))
>>>                      (current-buffer))
>>
>> It seems to me that the issue is related to the `and' being wrapped by
>> an `or', specifically because of a typo in the handling of `and':
>>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index 2ffc594997..0350c16ccf 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -7014,8 +7014,8 @@ buffer-match-p
>>                        (funcall match (cdr condition)))
>>                       ((eq (car-safe condition) 'and)
>>                        (catch 'fail
>> -                        (dolist (c (cdr conditions))
>> -                          (unless (funcall match c)
>> +                        (dolist (c (cdr condition))
>> +                          (unless (funcall match (list c))
>>                              (throw 'fail nil)))
>>                          t)))
>>                  (throw 'match t)))))))
>>
>>
>> As you have pointed out to me privately, it might make sense to rewrite
>> the case distinction using pcase, to avoid simple mistakes like these.
>
> That might look something like this:
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 2ffc594997..db1dc25044 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -6992,32 +6992,32 @@ buffer-match-p
>          (lambda (conditions)
>            (catch 'match
>              (dolist (condition conditions)
> -              (when (cond
> -                     ((eq condition t))
> -                     ((stringp condition)
> -                      (string-match-p condition (buffer-name buffer)))
> -                     ((functionp condition)
> -                      (if (eq 1 (cdr (func-arity condition)))
> -                          (funcall condition buffer)
> -                        (funcall condition buffer arg)))
> -                     ((eq (car-safe condition) 'major-mode)
> -                      (eq
> -                       (buffer-local-value 'major-mode buffer)
> -                       (cdr condition)))
> -                     ((eq (car-safe condition) 'derived-mode)
> -                      (provided-mode-derived-p
> -                       (buffer-local-value 'major-mode buffer)
> -                       (cdr condition)))
> -                     ((eq (car-safe condition) 'not)
> -                      (not (funcall match (cdr condition))))
> -                     ((eq (car-safe condition) 'or)
> -                      (funcall match (cdr condition)))
> -                     ((eq (car-safe condition) 'and)
> -                      (catch 'fail
> -                        (dolist (c (cdr conditions))
> -                          (unless (funcall match c)
> -                            (throw 'fail nil)))
> -                        t)))
> +              (when (pcase condition
> +                      ('t t)
> +                      ((pred stringp)
> +                       (string-match-p condition (buffer-name buffer)))
> +                      ((pred functionp)
> +                       (if (eq 1 (cdr (func-arity condition)))
> +                           (funcall condition buffer)
> +                         (funcall condition buffer arg)))
> +                      (`(major-mode . ,mode)
> +                       (eq
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))
> +                      (`(derived-mode . ,mode)
> +                       (provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))
> +                      (`(not . cond)
> +                       (not (funcall match cond)))
> +                      (`(or . ,args)
> +                       (funcall match args))
> +                      (`(and . ,args)
> +                       (catch 'fail
> +                         (dolist (c args)
> +                           (unless (funcall match (list c))
> +                             (throw 'fail nil)))
> +                         t)))
>                  (throw 'match t)))))))
>      (funcall match (list condition))))

Are there any objections against applying this change?  From what I see
pcase is used elsewhere in seq, so it should be possible to recognise
and expand the macro, right?





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-09-03 11:04     ` Philip Kaludercic
@ 2022-09-03 11:10       ` Eli Zaretskii
  2022-09-03 11:19         ` Philip Kaludercic
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2022-09-03 11:10 UTC (permalink / raw)
  To: Philip Kaludercic; +Cc: 57502, arstoffel

> Cc: 57502@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 03 Sep 2022 11:04:20 +0000
> 
> Are there any objections against applying this change?  From what I see
> pcase is used elsewhere in seq, so it should be possible to recognise
> and expand the macro, right?

I think we should avoid using pcase in subr.el, since subr.el is
loaded by loadup.el before pcase.  But I see that this ship has sailed
already, sigh.





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-09-03 11:10       ` Eli Zaretskii
@ 2022-09-03 11:19         ` Philip Kaludercic
  2022-09-03 11:54           ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: Philip Kaludercic @ 2022-09-03 11:19 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 57502, arstoffel

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57502@debbugs.gnu.org
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sat, 03 Sep 2022 11:04:20 +0000
>> 
>> Are there any objections against applying this change?  From what I see
>> pcase is used elsewhere in seq, so it should be possible to recognise
>> and expand the macro, right?
>
> I think we should avoid using pcase in subr.el, since subr.el is
> loaded by loadup.el before pcase.  But I see that this ship has sailed
> already, sigh.

There is no insistence from my end to use pcase, especially because at
first I assumed it ought not to be used in subr.el.  But from what I see
there have been instances of the macro (in `called-interactively-p') for
almost ten years now.  So unless there is a plan to revert this trend,
I'd use pcase to avoid simple issues like the one that caused this bug.





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-09-03 11:19         ` Philip Kaludercic
@ 2022-09-03 11:54           ` Lars Ingebrigtsen
  2022-09-03 12:56             ` Philip Kaludercic
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-09-03 11:54 UTC (permalink / raw)
  To: Philip Kaludercic; +Cc: 57502, Eli Zaretskii, arstoffel

Philip Kaludercic <philipk@posteo.net> writes:

> So unless there is a plan to revert this trend, I'd use pcase to avoid
> simple issues like the one that caused this bug.

Looks good to me.





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

* bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
  2022-09-03 11:54           ` Lars Ingebrigtsen
@ 2022-09-03 12:56             ` Philip Kaludercic
  0 siblings, 0 replies; 10+ messages in thread
From: Philip Kaludercic @ 2022-09-03 12:56 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 57502-done, Eli Zaretskii, arstoffel

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> So unless there is a plan to revert this trend, I'd use pcase to avoid
>> simple issues like the one that caused this bug.
>
> Looks good to me.

Ok, I've pushed the change.





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

end of thread, other threads:[~2022-09-03 12:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-31 12:02 bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p Augusto Stoffel
2022-08-31 12:47 ` Philip Kaludercic
2022-08-31 12:50   ` Philip Kaludercic
2022-08-31 16:22     ` Augusto Stoffel
2022-08-31 16:30       ` Philip Kaludercic
2022-09-03 11:04     ` Philip Kaludercic
2022-09-03 11:10       ` Eli Zaretskii
2022-09-03 11:19         ` Philip Kaludercic
2022-09-03 11:54           ` Lars Ingebrigtsen
2022-09-03 12:56             ` Philip Kaludercic

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