GNU bug report logs - #45226
Avoid redundant calls in narrow-to-defun

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45226 in the body.
You can then email your comments to 45226 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tomas Nordin <tomasn <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Avoid redundant calls in narrow-to-defun
Date: Sun, 13 Dec 2020 19:16:24 +0100

[Message part 1 (text/plain, inline)]

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 <at> 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

[narrow-to-defun.patch (text/x-diff, inline)]

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

Message #12 received at 45226 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tomas Nordin <tomasn <at> posteo.net>
Cc: 45226 <at> debbugs.gnu.org
Subject: Re: bug#45226: Avoid redundant calls in narrow-to-defun
Date: Thu, 11 Nov 2021 06:57:59 +0100

Tomas Nordin <tomasn <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.