* bug#45226: Avoid redundant calls in narrow-to-defun
@ 2020-12-13 18:16 Tomas Nordin
2021-11-11 5:57 ` Lars Ingebrigtsen
0 siblings, 1 reply; 2+ messages in thread
From: Tomas Nordin @ 2020-12-13 18:16 UTC (permalink / raw)
To: 45226
[-- Attachment #1: Type: text/plain, Size: 3949 bytes --]
Hello Emacs
Working with bug 40563 I had reason "demystify" the narrow-to-defun
function and I think one can see a redundant check and call to
beginning-of-defun there. Here is a walk-through of the beginning of the
narrow-to-defun function to try and explain what I mean.
...
(let ((opoint (point))
beg end)
;; Try first in this order for the sake of languages with nested
;; functions where several can end at the same place as with
;; the offside rule, e.g. Python.
;; Finding the start of the function is a bit problematic since
;; `beginning-of-defun' when we are on the first character of
;; the function might go to the previous function.
;;
;; Therefore we first move one character forward and then call
;; `beginning-of-defun'. However now we must check that we did
;; not move into the next function.
(let ((here (point)))
(unless (eolp)
(forward-char))
forward-char to allow narrowing with point on the entry of a defun
(allow the beginning-of-defun function to find the beginning of /this/
defun).
(beginning-of-defun)
(when (< (point) here)
This test pass in all cases but the case when point was on the entry of a
defun at calling narrow-to-defun.
(goto-char here)
(beginning-of-defun)))
And then beginning-of-defun function is called again, with point on
'here', from where we started.
(setq beg (point))
beg is now possibly the result from the second call to
beginning-of-defun.
In the case of lisp defuns (defuns starting at beginning-of-line) this
is no problem, only redundant I would claim (the test for (< (point)
here) and the additional call to beginning-of-defun). With a language
with the mentioned "offside rule", like Python, it might be a problem.
If the intention was to narrow a nested function with point on the the
first character of the defun, the following happens, (| is point)
class A():
|def a1(self):
pass
The forward-char call allow beginning-of-defun function to find the
beginning of a1. If not elsewhere, the beginning-of-defun function in
lisp.el finalize with beginning-of-line, so we get
class A():
| def a1(self):
pass
and that means (< (point) here) and beginning-of-defun is called again,
this time from the beginning of def ('here') and no forward-char
support, ending up at the beginning of class A. Other funny things can
happen but I stop there.
It is not a problem with lisp because a defun start at
beginning-of-line.
In any case, I cannot see what problem the forward-char could cause, and
the protection for it. The nested let with the forward-char service was
introduced with
commit 050cc68b402f5998193a6026d0eeeecb9d2cb9c4
Author: Lennart Borgman <lennart.borgman@gmail.com>
Date: Wed Apr 11 04:12:20 2012 +0200
`narrow-to-defun' fixup
* emacs-lisp/lisp.el (narrow-to-defun): `beginning-of-defun' goes
to previous function when point is on the first character of a
function. Take care of that in `narrow-to-defun'.
Fixes: debbugs:6157
and in the discussion about it (bug 6157) I cannot see an example of
where it would be a problem. The comment is saying we must check we
didn't end up in the next function, but that's not what is checked in
that 'when' test as far as I understand.
With the attached patch I suggest a partial revert of the above commit,
keeping only the forward-char service. I have been experimenting with it
a lot and see no problem. I made 'make check' and see no failures FWIW.
As a bonus, in combination with a patch I submitted with bug#40563,
python nested defuns can be narrowed.
What do you think, maybe I am missing something. Or do you agree the
check and the additional call is redundant?
GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
cairo version 1.16.0) of 2020-12-12
lisp.el(c) commit 8ace7700b9
Best regards
--
Tomas
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: narrow-to-defun.patch --]
[-- Type: text/x-diff, Size: 820 bytes --]
diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 124900168c..83c20933b3 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -659,15 +659,10 @@ narrow-to-defun
;; the function might go to the previous function.
;;
;; Therefore we first move one character forward and then call
- ;; `beginning-of-defun'. However now we must check that we did
- ;; not move into the next function.
- (let ((here (point)))
- (unless (eolp)
- (forward-char))
- (beginning-of-defun)
- (when (< (point) here)
- (goto-char here)
- (beginning-of-defun)))
+ ;; `beginning-of-defun'.
+ (unless (eolp)
+ (forward-char))
+ (beginning-of-defun)
(setq beg (point))
(end-of-defun)
(setq end (point))
^ permalink raw reply related [flat|nested] 2+ messages in thread
* bug#45226: Avoid redundant calls in narrow-to-defun
2020-12-13 18:16 bug#45226: Avoid redundant calls in narrow-to-defun Tomas Nordin
@ 2021-11-11 5:57 ` Lars Ingebrigtsen
0 siblings, 0 replies; 2+ messages in thread
From: Lars Ingebrigtsen @ 2021-11-11 5:57 UTC (permalink / raw)
To: Tomas Nordin; +Cc: 45226
Tomas Nordin <tomasn@posteo.net> writes:
> `narrow-to-defun' fixup
>
> * emacs-lisp/lisp.el (narrow-to-defun): `beginning-of-defun' goes
> to previous function when point is on the first character of a
> function. Take care of that in `narrow-to-defun'.
>
[...]
> With the attached patch I suggest a partial revert of the above commit,
> keeping only the forward-char service. I have been experimenting with it
> a lot and see no problem. I made 'make check' and see no failures FWIW.
> As a bonus, in combination with a patch I submitted with bug#40563,
> python nested defuns can be narrowed.
>
> What do you think, maybe I am missing something. Or do you agree the
> check and the additional call is redundant?
I think the use case is if you have a language where you can define
several function definitions on the same line. Like something like:
(defun foo ())(defun bar ())(defun zot ())
Unfortunately the original bug report didn't have a case for
reproduction, so I don't know what the language in question where the
problem may have been in. (There should have been a test case, of
course.)
(And in Emacs Lisp, this doesn't work anyway)
So I'd prefer not to revert the change (it's a defensive change), unless
we know what it's not really doing anything useful. So I'm closing this
bug report.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-11 5:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-13 18:16 bug#45226: Avoid redundant calls in narrow-to-defun Tomas Nordin
2021-11-11 5:57 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.