* pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
@ 2024-05-26 15:30 Alan Mackenzie
2024-05-26 21:19 ` Michael Heerdegen via Emacs development discussions.
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Alan Mackenzie @ 2024-05-26 15:30 UTC (permalink / raw)
To: emacs-devel; +Cc: Stefan Monnier
Hello, Stefan and Emacs.
The ert test erc--restore-initialize-priors in test/lisp/erc/erc-tests.el
looks like this:
(ert-deftest erc--restore-initialize-priors ()
(unless (>= emacs-major-version 28)
(ert-skip "Lisp nesting exceeds `max-lisp-eval-depth'"))
(should (pcase (macroexpand-1 '(erc--restore-initialize-priors erc-my-mode
foo (ignore 1 2 3)
bar #'spam
baz nil))
(`(let* ((,p (or erc--server-reconnecting erc--target-priors))
(,q (and ,p (alist-get 'erc-my-mode ,p))))
(unless (local-variable-if-set-p 'erc-my-mode)
(error "Not a local minor mode var: %s" 'erc-my-mode))
(setq foo (if ,q (alist-get 'foo ,p) (ignore 1 2 3))
bar (if ,q (alist-get 'bar ,p) #'spam)
baz (if ,q (alist-get 'baz ,p) nil)))
t))))
.. Note that the pcase form is expanding a backquoted form lacking in
pcase features such as nested backquotes, pcase variable names, and the
like.
The pcase expansion thus consists of a deeply nested alternation of forms
like
(if (consp x1961) ...)
and
(let* ((x1962 (car-safe x1961))) ...)
..
The level of nesting is greater than, or close enough to 200 that
printing it can lead to the detection of an apparent circular structure
in print_object in src/print.c. There PRINT_CIRCLE is #defined as 200.
This leads the printer to printing out the error message:
Apparently circular structure being printed
and throwing an error. When this happened to me, the ert structure
containing this expansion was accessible from the stack, so it triggered
the error again recursively, resulting in the log file erc-tests.log
being 131 MB big. I haven't looked to see what terminated this recursive
exception.
The situation seems to be what triggered bug#71178.
The form being compared using pcase, although not tiny, is not all that
big, and it would be easy to increase its size to cause it to violate any
reasonable value of PRINT_CIRCLE.
Would it be possible and a good idea to amend pcase such that it
generates less deeply nested expansions for forms such as we have here?
Or does anybody have any ideas how better to resolve the problem?
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-26 15:30 pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors Alan Mackenzie
@ 2024-05-26 21:19 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 17:39 ` Stefan Monnier
2024-05-30 23:45 ` Stefan Monnier
2 siblings, 0 replies; 15+ messages in thread
From: Michael Heerdegen via Emacs development discussions. @ 2024-05-26 21:19 UTC (permalink / raw)
To: emacs-devel
Alan Mackenzie <acm@muc.de> writes:
> The form being compared using pcase, although not tiny, is not all that
> big, and it would be easy to increase its size to cause it to violate any
> reasonable value of PRINT_CIRCLE.
Even worse: if you try to compile a file consisting of only this defun
(this is exactly one part of that test factored out):
#+begin_src emacs-lisp
(defun erc--restore-initialize-priors-1 (expansion)
(pcase expansion
(`(let* ((,p (or erc--server-reconnecting erc--target-priors))
(,q (and ,p (alist-get 'erc-my-mode ,p))))
(unless (local-variable-if-set-p 'erc-my-mode)
(error "Not a local minor mode var: %s" 'erc-my-mode))
(setq foo (if ,q (alist-get 'foo ,p) (ignore 1 2 3))
bar (if ,q (alist-get 'bar ,p) #'spam)
baz (if ,q (alist-get 'baz ,p) nil)))
t)))
#+end_src
with a slightly reduced `max-lisp-eval-depth' (from 1600 to 1000)
compilation fails.
> Would it be possible and a good idea to amend pcase such that it
> generates less deeply nested expansions for forms such as we have here?
I think this todo in pcase.el is related:
;; - try and be more clever to reduce the size of the decision tree, and
;; to reduce the number of leaves that need to be turned into functions:
> Or does anybody have any ideas how better to resolve the problem?
Apart from trying to work around it somehow, I don't.
Michael.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-26 15:30 pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors Alan Mackenzie
2024-05-26 21:19 ` Michael Heerdegen via Emacs development discussions.
@ 2024-05-27 17:39 ` Stefan Monnier
2024-05-27 18:23 ` Stefan Monnier
2024-05-27 20:14 ` F. Jason Park
2024-05-30 23:45 ` Stefan Monnier
2 siblings, 2 replies; 15+ messages in thread
From: Stefan Monnier @ 2024-05-27 17:39 UTC (permalink / raw)
To: Alan Mackenzie; +Cc: emacs-devel
> (should (pcase (macroexpand-1 '(erc--restore-initialize-priors erc-my-mode
> foo (ignore 1 2 3)
> bar #'spam
> baz nil))
> (`(let* ((,p (or erc--server-reconnecting erc--target-priors))
> (,q (and ,p (alist-get 'erc-my-mode ,p))))
> (unless (local-variable-if-set-p 'erc-my-mode)
> (error "Not a local minor mode var: %s" 'erc-my-mode))
> (setq foo (if ,q (alist-get 'foo ,p) (ignore 1 2 3))
> bar (if ,q (alist-get 'bar ,p) #'spam)
> baz (if ,q (alist-get 'baz ,p) nil)))
> t))))
Hmm...
Looking at it from a `pcase.el` perspective, the above pattern represents
a really large number of conditions :-)
> .. Note that the pcase form is expanding a backquoted form lacking in
> pcase features such as nested backquotes, pcase variable names, and the
> like.
[ I didn't understand what you meant here, sorry. ]
> The pcase expansion thus consists of a deeply nested alternation of forms
> like
> (if (consp x1961) ...)
> and
> (let* ((x1962 (car-safe x1961))) ...)
> ..
Oh, yes!
> The level of nesting is greater than, or close enough to 200 that
> printing it can lead to the detection of an apparent circular structure
> in print_object in src/print.c. There PRINT_CIRCLE is #defined as 200.
I'm surprised it doesn't also cause errors during compilation (because
of too deeply nested recursive calls).
> The form being compared using pcase, although not tiny, is not all that
> big, and it would be easy to increase its size to cause it to violate any
> reasonable value of PRINT_CIRCLE.
Agreed.
> Would it be possible and a good idea to amend pcase such that it
> generates less deeply nested expansions for forms such as we have here?
A good idea? Definitely.
Possible? No doubt.
Not sure how easy it would be, OTOH.
FWIW, this is a case where the compilation strategy Richard uses in
`cond*` probably works better.
I think the main thing to do here would be to see how to recognize
subpatterns that don't have any `,` in them so we can match them with
a simple `equal` test. E.g. the whole
(unless (local-variable-if-set-p 'erc-my-mode)
(error "Not a local minor mode var: %s" 'erc-my-mode))
subpattern can be matched much more efficiently with a single `equal`,
I suspect it can be done with no change to the core pcase code by
rewriting
(pcase-defmacro \` (qpat)
so it does the recursion on its own (e.g. instead of expanding `(A . B)
to a pattern which contains `A and `B). Then at any level if it notices
that no `,` was found during the recursion it can just use `equal`.
Another approach would be to change the "back end" of pcase so that when
we built the nested `let*/if` monster we recognize specific patterns
that can be rewritten more compactly.
> Or does anybody have any ideas how better to resolve the problem?
FWIW, I'm wondering what's the value of this specific test.
Do we really care about the specific shape of the returned code?
It seems terribly brittle: every minute change to the macro will require
matching changes to the test, and I have no idea what is the intention
of this test so if it every fails I wouldn't know whether the error is
in the test or in the code.
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 17:39 ` Stefan Monnier
@ 2024-05-27 18:23 ` Stefan Monnier
2024-05-27 19:29 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 20:14 ` F. Jason Park
1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2024-05-27 18:23 UTC (permalink / raw)
To: Alan Mackenzie; +Cc: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 476 bytes --]
> I suspect it can be done with no change to the core pcase code by
> rewriting
>
> (pcase-defmacro \` (qpat)
>
> so it does the recursion on its own (e.g. instead of expanding `(A . B)
> to a pattern which contains `A and `B). Then at any level if it notices
> that no `,` was found during the recursion it can just use `equal`.
The patch below does that.
For that ERC test, the resulting code is still very heavily nested, of
course, but ... less so.
Stefan
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pcase.patch --]
[-- Type: text/x-diff, Size: 2056 bytes --]
diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index 1a58c60734a..bbc08493ec9 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -1124,21 +1124,35 @@ \`
- True! (The second element can be anything, and for the sake
of the body forms, its value is bound to the symbol `forum'.)"
(declare (debug (pcase-QPAT)))
+ (pcase--expand-\` qpat))
+
+(defun pcase--expand-\` (qpat)
(cond
((eq (car-safe qpat) '\,) (cadr qpat))
((eq (car-safe qpat) '\,@) (error "Unsupported QPAT: %S" qpat))
((vectorp qpat)
- `(and (pred vectorp)
- (app length ,(length qpat))
- ,@(let ((upats nil))
- (dotimes (i (length qpat))
- (push `(app (aref _ ,i) ,(list '\` (aref qpat i)))
- upats))
- (nreverse upats))))
+ (let* ((trivial t)
+ (upats (mapcar (lambda (qpat)
+ (let ((upat (pcase--expand-\` qpat)))
+ (unless (eq (car-safe upat) 'quote)
+ (setq trivial nil))
+ upat))
+ qpat)))
+ (if trivial
+ `',qpat
+ `(and (pred vectorp)
+ (app length ,(length qpat))
+ ,@(let ((i -1))
+ (mapcar (lambda (upat) `(app (aref _ ,(setq i (+ i 1))) ,upat))
+ upats))))))
((consp qpat)
- `(and (pred consp)
- (app car-safe ,(list '\` (car qpat)))
- (app cdr-safe ,(list '\` (cdr qpat)))))
+ (let ((upata (pcase--expand-\` (car qpat)))
+ (upatd (pcase--expand-\` (cdr qpat))))
+ (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
+ `',qpat
+ `(and (pred consp)
+ (app car-safe ,upata)
+ (app cdr-safe ,upatd)))))
((or (stringp qpat) (numberp qpat) (symbolp qpat)) `',qpat)
;; In all other cases just raise an error so we can't break
;; backward compatibility when adding \` support for other
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 18:23 ` Stefan Monnier
@ 2024-05-27 19:29 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 19:42 ` Stefan Monnier
0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen via Emacs development discussions. @ 2024-05-27 19:29 UTC (permalink / raw)
To: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> The patch below does that.
Thanks also from my side for working on this.
> +(defun pcase--expand-\` (qpat)
> [...]
> ((consp qpat)
> - `(and (pred consp)
> - (app car-safe ,(list '\` (car qpat)))
> - (app cdr-safe ,(list '\` (cdr qpat)))))
> + (let ((upata (pcase--expand-\` (car qpat)))
> + (upatd (pcase--expand-\` (cdr qpat))))
> + (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
> + `',qpat
> + [...]
I wondered whether this test is too simple. AFAIU, at least this
slightly obscure case gets broken:
(pcase '(a . b)
(`(a . ,'b) t))
Michael.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 19:29 ` Michael Heerdegen via Emacs development discussions.
@ 2024-05-27 19:42 ` Stefan Monnier
2024-05-29 15:39 ` Michael Heerdegen
0 siblings, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2024-05-27 19:42 UTC (permalink / raw)
To: Michael Heerdegen via Emacs development discussions.; +Cc: Michael Heerdegen
> I wondered whether this test is too simple. AFAIU, at least this
> slightly obscure case gets broken:
>
> (pcase '(a . b)
> (`(a . ,'b) t))
The code I sent wasn't quite right, indeed. 🙂
[ I didn't think of it as a failure of the test, but a failure of the
code which (re)constructs the resulting quote pattern. ]
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 19:42 ` Stefan Monnier
@ 2024-05-29 15:39 ` Michael Heerdegen
2024-05-30 23:43 ` Stefan Monnier
2024-06-03 17:42 ` Stefan Monnier
0 siblings, 2 replies; 15+ messages in thread
From: Michael Heerdegen @ 2024-05-29 15:39 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Michael Heerdegen via Emacs development discussions.
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> > (pcase '(a . b)
> > (`(a . ,'b) t))
>
> The code I sent wasn't quite right, indeed. 🙂
> [ I didn't think of it as a failure of the test, but a failure of the
> code which (re)constructs the resulting quote pattern. ]
Anyway, apart from this little detail the patch is an obvious
improvement, so please go ahead. The occupied lisp eval depth shrinks
around 50% when the `pcase' pattern in the originally posted ert test is
compiled.
I must admit however - I still wonder: Code and matched structures will
be proper short-length lists most of the time - would it be possible and
make sense to handle all list elements at the same recursion level
instead of doing O(length) recursive descents?
Michael.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-29 15:39 ` Michael Heerdegen
@ 2024-05-30 23:43 ` Stefan Monnier
2024-05-31 7:32 ` Andrea Corallo
2024-06-03 17:42 ` Stefan Monnier
1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2024-05-30 23:43 UTC (permalink / raw)
To: Michael Heerdegen; +Cc: Michael Heerdegen via Emacs development discussions.
> I must admit however - I still wonder: Code and matched structures will
> be proper short-length lists most of the time - would it be possible and
> make sense to handle all list elements at the same recursion level
> instead of doing O(length) recursive descents?
I think if you look at the bytecode you'll see it uses a lot less
stack space, so the problem is not as serious as it seems.
Apart from that, for the specific case of matching against just one
pattern, we could do better. As mentioned earlier, that's what
`match*` does.
In contrast `pcase.el` is designed to solve the more general case of
pattern matching against several patterns at the same time with the goal
of avoiding repeating the same operation across the various patterns.
See below the code that `match*` generates. As you can see, it's much
less nested (but it does a lot of redundant computations even for this
one pattern).
[ Note also that the code generated by `pcase.el` (without my patch),
while much more deeply nested, is only about twice the size of the
code generated by `match*`. With my patch, it is almost the same size
(about 10% larger still). I measured the size via `prin1-to-string`
and get about the same relative results when looking at interpreted
code and the byte-compiled code.
So, I'm quite happy with `pcase.el` performance for this case. ]
Stefan
ELISP> (macroexpand '(cond* ((match* `(let* ((,p (or erc--server-reconnecting erc--target-priors))
(,q (and ,p (alist-get 'erc-my-mode ,p))))
(unless (local-variable-if-set-p 'erc-my-mode)
(error "Not a local minor mode var: %s" 'erc-my-mode))
(setq foo (if ,q (alist-get 'foo ,p) (ignore 1 2 3))
bar (if ,q (alist-get 'bar ,p) #'spam)
baz (if ,q (alist-get 'baz ,p) nil))) EXP) RES)))
(let ((d2 EXP))
(if
(and (consp d2) (eq 'let* (nth 0 d2)) (consp (nthcdr 1 d2))
(and (consp (nth 1 d2))
(and (consp (nth 0 (nth 1 d2)))
(equal
'((or erc--server-reconnecting erc--target-priors))
(cdr (nth 0 (nth 1 d2)))))
(consp (nthcdr 1 (nth 1 d2)))
(and (consp (nth 1 (nth 1 d2)))
(consp (nthcdr 1 (nth 1 (nth 1 d2))))
(and (consp (nth 1 (nth 1 (nth 1 d2))))
(eq 'and (nth 0 (nth 1 (nth 1 (nth 1 d2)))))
(consp (nthcdr 1 (nth 1 (nth 1 (nth 1 d2)))))
(equal (car (nth 0 (nth 1 d2)))
(nth 1 (nth 1 (nth 1 (nth 1 d2)))))
(consp (nthcdr 2 (nth 1 (nth 1 (nth 1 d2)))))
(and (consp (nth 2 (nth 1 (nth 1 (nth 1 d2)))))
(eq 'alist-get
(nth 0
(nth 2 (nth 1 (nth 1 (nth 1 d2))))))
(consp
(nthcdr 1
(nth 2 (nth 1 (nth 1 (nth 1 d2))))))
(equal ''erc-my-mode
(nth 1
(nth 2
(nth 1 (nth 1 (nth 1 d2))))))
(consp
(nthcdr 2
(nth 2 (nth 1 (nth 1 (nth 1 d2))))))
(equal (car (nth 0 (nth 1 d2)))
(nth 2
(nth 2
(nth 1 (nth 1 (nth 1 d2))))))
(null
(nthcdr 3
(nth 2 (nth 1 (nth 1 (nth 1 d2)))))))
(null (nthcdr 3 (nth 1 (nth 1 (nth 1 d2))))))
(null (nthcdr 2 (nth 1 (nth 1 d2)))))
(null (nthcdr 2 (nth 1 d2))))
(consp (nthcdr 2 d2))
(equal
'(unless (local-variable-if-set-p 'erc-my-mode)
(error "Not a local minor mode var: %s" 'erc-my-mode))
(nth 2 d2))
(consp (nthcdr 3 d2))
(and (consp (nth 3 d2)) (eq 'setq (nth 0 (nth 3 d2)))
(consp (nthcdr 1 (nth 3 d2))) (eq 'foo (nth 1 (nth 3 d2)))
(consp (nthcdr 2 (nth 3 d2)))
(and (consp (nth 2 (nth 3 d2)))
(eq 'if (car (nth 2 (nth 3 d2))))
(and (consp (cdr (nth 2 (nth 3 d2))))
(equal (nth 0 (nth 1 (nth 1 d2)))
(car (cdr (nth 2 (nth 3 d2)))))
(and (consp (cdr (cdr (nth 2 (nth 3 d2)))))
(and
(consp
(car (cdr (cdr (nth 2 (nth 3 d2))))))
(eq 'alist-get
(nth 0
(car
(cdr (cdr (nth 2 (nth 3 d2)))))))
(consp
(nthcdr 1
(car
(cdr (cdr (nth 2 (nth 3 d2)))))))
(equal ''foo
(nth 1
(car
(cdr (cdr (nth 2 (nth 3 d2)))))))
(consp
(nthcdr 2
(car
(cdr (cdr (nth 2 (nth 3 d2)))))))
(equal (car (nth 0 (nth 1 d2)))
(nth 2
(car
(cdr (cdr (nth 2 (nth 3 d2)))))))
(null
(nthcdr 3
(car
(cdr (cdr (nth 2 (nth 3 d2))))))))
(equal '((ignore 1 2 3))
(cdr (cdr (cdr (nth 2 (nth 3 d2)))))))))
(consp (nthcdr 3 (nth 3 d2))) (eq 'bar (nth 3 (nth 3 d2)))
(consp (nthcdr 4 (nth 3 d2)))
(and (consp (nth 4 (nth 3 d2)))
(eq 'if (car (nth 4 (nth 3 d2))))
(and (consp (cdr (nth 4 (nth 3 d2))))
(equal (nth 0 (nth 1 (nth 1 d2)))
(car (cdr (nth 4 (nth 3 d2)))))
(and (consp (cdr (cdr (nth 4 (nth 3 d2)))))
(and
(consp
(car (cdr (cdr (nth 4 (nth 3 d2))))))
(eq 'alist-get
(nth 0
(car
(cdr (cdr (nth 4 (nth 3 d2)))))))
(consp
(nthcdr 1
(car
(cdr (cdr (nth 4 (nth 3 d2)))))))
(equal ''bar
(nth 1
(car
(cdr (cdr (nth 4 (nth 3 d2)))))))
(consp
(nthcdr 2
(car
(cdr (cdr (nth 4 (nth 3 d2)))))))
(equal (car (nth 0 (nth 1 d2)))
(nth 2
(car
(cdr (cdr (nth 4 (nth 3 d2)))))))
(null
(nthcdr 3
(car
(cdr (cdr (nth 4 (nth 3 d2))))))))
(equal '(#'spam)
(cdr (cdr (cdr (nth 4 (nth 3 d2)))))))))
(consp (nthcdr 5 (nth 3 d2))) (eq 'baz (nth 5 (nth 3 d2)))
(consp (nthcdr 6 (nth 3 d2)))
(and (consp (nth 6 (nth 3 d2)))
(eq 'if (car (nth 6 (nth 3 d2))))
(and (consp (cdr (nth 6 (nth 3 d2))))
(equal (nth 0 (nth 1 (nth 1 d2)))
(car (cdr (nth 6 (nth 3 d2)))))
(and (consp (cdr (cdr (nth 6 (nth 3 d2)))))
(and
(consp
(car (cdr (cdr (nth 6 (nth 3 d2))))))
(eq 'alist-get
(nth 0
(car
(cdr (cdr (nth 6 (nth 3 d2)))))))
(consp
(nthcdr 1
(car
(cdr (cdr (nth 6 (nth 3 d2)))))))
(equal ''baz
(nth 1
(car
(cdr (cdr (nth 6 (nth 3 d2)))))))
(consp
(nthcdr 2
(car
(cdr (cdr (nth 6 (nth 3 d2)))))))
(equal (car (nth 0 (nth 1 d2)))
(nth 2
(car
(cdr (cdr (nth 6 (nth 3 d2)))))))
(null
(nthcdr 3
(car
(cdr (cdr (nth 6 (nth 3 d2))))))))
(equal '(nil)
(cdr (cdr (cdr (nth 6 (nth 3 d2)))))))))
(null (nthcdr 7 (nth 3 d2))))
(null (nthcdr 4 d2)))
(let*
((q (nth 0 (nth 1 (nth 1 d2)))) (p (car (nth 0 (nth 1 d2)))))
RES)
nil))
ELISP>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-30 23:43 ` Stefan Monnier
@ 2024-05-31 7:32 ` Andrea Corallo
2024-05-31 12:45 ` Stefan Monnier
0 siblings, 1 reply; 15+ messages in thread
From: Andrea Corallo @ 2024-05-31 7:32 UTC (permalink / raw)
To: Stefan Monnier
Cc: Michael Heerdegen,
Michael Heerdegen via Emacs development discussions.
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I must admit however - I still wonder: Code and matched structures will
>> be proper short-length lists most of the time - would it be possible and
>> make sense to handle all list elements at the same recursion level
>> instead of doing O(length) recursive descents?
>
> I think if you look at the bytecode you'll see it uses a lot less
> stack space, so the problem is not as serious as it seems.
True, just to mention that OTOH code running during bootstrap is
impacted, native comp as you know is a pcase aficionado and in the past
and in some of my experimental branches I encountered this limit.
Andrea
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-29 15:39 ` Michael Heerdegen
2024-05-30 23:43 ` Stefan Monnier
@ 2024-06-03 17:42 ` Stefan Monnier
2024-06-04 0:57 ` Michael Heerdegen
1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2024-06-03 17:42 UTC (permalink / raw)
To: Michael Heerdegen; +Cc: Michael Heerdegen via Emacs development discussions.
> Anyway, apart from this little detail the patch is an obvious
> improvement, so please go ahead.
Well, there were some interactions with other pcase optimizations which
made it sometimes generate worse code. I think I got this mostly fixed
now (not completely, but that's the world of compiler optimization for
you), so I just pushed it to `master`.
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 17:39 ` Stefan Monnier
2024-05-27 18:23 ` Stefan Monnier
@ 2024-05-27 20:14 ` F. Jason Park
2024-05-27 22:31 ` Stefan Monnier
1 sibling, 1 reply; 15+ messages in thread
From: F. Jason Park @ 2024-05-27 20:14 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Alan Mackenzie, emacs-devel
Hi Stefan,
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> FWIW, I'm wondering what's the value of this specific test.
> Do we really care about the specific shape of the returned code?
No, we don't care. This test is indeed useless and will be removed.
> It seems terribly brittle: every minute change to the macro will require
> matching changes to the test, and I have no idea what is the intention
> of this test so if it every fails I wouldn't know whether the error is
> in the test or in the code.
A test _like_ this might make some sense in a case where the code under
test is itself deceptively brittle due to being tightly coupled to a
dependency, such as the specific shape of another, more complicated
macro whose expansion it modifies in ways that aren't easily detectable
through behavior alone [1]. In such cases, copying over ("vendoring")
the entirety of the complicated "upstream" macro's definition merely to
tweak minor aspects, presumably ones that can't easily be overridden or
redefined after evaluation, due to side effects, may be deemed a bigger
maintenance burden. IOW, after weighing the trade offs, it may be
decided such modifications would be better handled through brittle
twiddling/surgery during expansion. The point of such a test [2] would
then be to "fail fast" so as to alert you to upstream's changes as they
happen. Obviously, such a thing should not be done across ownership
boundaries. And better practices, like breaking up the complicated
dependency into reusable pieces, would be more sustainable long term.
Thanks,
J.P.
[1] https://gitlab.com/emacs-erc/edge/-/blob/fa37e9945df07e8bdb377bd5f5f30d6543521d1e/erc-common.el#L841
[2] https://gitlab.com/emacs-erc/edge/-/blob/fa37e9945df07e8bdb377bd5f5f30d6543521d1e/test/erc-tests.el#L3888
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-27 20:14 ` F. Jason Park
@ 2024-05-27 22:31 ` Stefan Monnier
0 siblings, 0 replies; 15+ messages in thread
From: Stefan Monnier @ 2024-05-27 22:31 UTC (permalink / raw)
To: F. Jason Park; +Cc: Alan Mackenzie, emacs-devel
>> It seems terribly brittle: every minute change to the macro will require
>> matching changes to the test, and I have no idea what is the intention
>> of this test so if it every fails I wouldn't know whether the error is
>> in the test or in the code.
> A test _like_ this might make some sense in a case where [...]
Indeed, never say never. But then the test should come with a comment
explaining what's going on so tht when it fails we have a fighting
chance of deciding who's to blame and how to fix it.
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors
2024-05-26 15:30 pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors Alan Mackenzie
2024-05-26 21:19 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 17:39 ` Stefan Monnier
@ 2024-05-30 23:45 ` Stefan Monnier
2 siblings, 0 replies; 15+ messages in thread
From: Stefan Monnier @ 2024-05-30 23:45 UTC (permalink / raw)
To: Alan Mackenzie; +Cc: emacs-devel
> The form being compared using pcase, although not tiny, is not all that
> big, and it would be easy to increase its size to cause it to violate any
> reasonable value of PRINT_CIRCLE.
I understand there can be some downsides, but maybe activating
`print-circle` would be a good way to avoid this `PRINT_CIRCLE`
hardcoded limit?
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-06-04 0:57 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-26 15:30 pcase generates an unprintable expansion for a form in test erc--restore-initialize-priors Alan Mackenzie
2024-05-26 21:19 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 17:39 ` Stefan Monnier
2024-05-27 18:23 ` Stefan Monnier
2024-05-27 19:29 ` Michael Heerdegen via Emacs development discussions.
2024-05-27 19:42 ` Stefan Monnier
2024-05-29 15:39 ` Michael Heerdegen
2024-05-30 23:43 ` Stefan Monnier
2024-05-31 7:32 ` Andrea Corallo
2024-05-31 12:45 ` Stefan Monnier
2024-06-03 17:42 ` Stefan Monnier
2024-06-04 0:57 ` Michael Heerdegen
2024-05-27 20:14 ` F. Jason Park
2024-05-27 22:31 ` Stefan Monnier
2024-05-30 23:45 ` Stefan Monnier
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).