[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#45226: Avoid redundant calls in narrow-to-defun
From: |
Tomas Nordin |
Subject: |
bug#45226: Avoid redundant calls in narrow-to-defun |
Date: |
Sun, 13 Dec 2020 19:16:24 +0100 |
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
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))
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- bug#45226: Avoid redundant calls in narrow-to-defun,
Tomas Nordin <=