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