Brave new mark-defun (and a testing tool)

Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

Hi all,

after several months of on-and-off work on bug#21072, I have implemented
two (hopefully) nice features.  One is an (almost) completely new
version of mark-defun, which I hope works much better than the previous
one:

--8<---------------cut here---------------start------------->8---
(defun in-comment-line-p ()
  "Return non-nil if the point is in a comment line."
;; See http://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00141.html
  (save-excursion
    (forward-line 0)
    (unless (looking-at "^\\s-*$")
      (< (line-end-position)
         (let ((ppss (syntax-ppss)))
           (when (nth 4 ppss)
             (goto-char (nth 8 ppss)))
           (forward-comment (point-max))
           (point))))))

(defun beginning-of-defun-comments (&optional arg)
  "Move to the beginning of ARGth defun, including comments."
  (interactive "^p")
  (unless arg (setq arg 1))
  (beginning-of-defun arg)
  (let (nbobp)
    (while (progn
             (setq nbobp (zerop (forward-line -1)))
             (and (in-comment-line-p)
                  nbobp)))
    (when nbobp
      (forward-line 1))))

(defun mark-defun (&optional arg)
  "Put mark at end of this defun, point at beginning.
The defun marked is the one that contains point or follows point.
With positive ARG, mark this and that many next defuns; with negative
ARG, change the direction of marking.

If the mark is active, it marks the next or previous defun(s) after
the one(s) already marked."
  (interactive "p")
  (setq arg (or arg 1))
  ;; There is no `mark-defun-back' function - see
  ;; https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-11/msg00079.html
  ;; for explanation
  (when (eq last-command 'mark-defun-back)
    (setq arg (- arg)))
  (when (< arg 0)
    (setq this-command 'mark-defun-back))
  (cond ((use-region-p)
         (if (>= arg 0)
             (set-mark
              (save-excursion
                (goto-char (mark))
                ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
                (dotimes (_ignore arg)
                  (end-of-defun))
                (point)))
           (beginning-of-defun-comments (- arg))))
        (t
         (let ((opoint (point))
               beg end)
           (push-mark opoint)
           ;; 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.
           (beginning-of-defun-comments)
           (setq beg (point))
           (end-of-defun)
           (setq end (point))
           (when (or (and (<= (point) opoint)
                          (> arg 0))
                     (= beg (point-min))) ; we were before the first defun!
             ;; beginning-of-defun moved back one defun so we got the wrong
             ;; one.  If ARG < 0, however, we actually want to go back.
             (goto-char opoint)
             (end-of-defun)
             (setq end (point))
             (beginning-of-defun-comments)
             (setq beg (point)))
           (goto-char beg)
           (cond ((> arg 0)
                  ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
                  (dotimes (_ignore arg)
                    (end-of-defun))
                  (setq end (point))
                  (push-mark end nil t)
                  (goto-char beg))
                 (t
                  (goto-char beg)
                  (unless (= arg -1)    ; beginning-of-defun behaves
                                        ; strange with zero arg - see
                                        ; https://lists.gnu.org/archive/html/bug-gnu-emacs/2017-02/msg00196.html
                    (beginning-of-defun (1- (- arg))))
                  (push-mark end nil t))))))
  (let (nbobp)
    (while (progn
             (setq nbobp (zerop (forward-line -1)))
             (and (looking-at "^\\s-*$")
                  nbobp)))
    (when nbobp
      (forward-line 1))))
--8<---------------cut here---------------end--------------->8---

Aside from that, I spent a considerable time writing ERT tests for that
function, and to that end I developed the macro below.  It's main goal
is to be able to specify contents of a test buffer together with a set
of named positions in that buffer, so that we can test commands that
move the point and/or mark around easily.

--8<---------------cut here---------------start------------->8---
(defvar elisp-test-point-marker-regex "=!\\([a-zA-Z0-9-]+\\)="
  "A regexp matching placeholders for point position for
`elisp-tests-with-temp-buffer'.")

;; Copied and heavily modified from `python-tests-with-temp-buffer'
(defmacro elisp-tests-with-temp-buffer (contents &rest body)
  "Create an `emacs-lisp-mode' enabled temp buffer with CONTENTS.
BODY is the code to be executed within the temp buffer.  Point is
always located at the beginning of buffer.  Special markers of
the form =!NAME= in CONTENTS are removed, and a for each one
a variable called NAME is bound to the position of such
a marker."
  (declare (indent 1) (debug t))
  `(with-temp-buffer
     (emacs-lisp-mode)
     (insert ,contents)
     (goto-char (point-min))
     (while (re-search-forward elisp-test-point-marker-regex nil t)
       (delete-region (match-beginning 0)
		      (match-end 0)))
     (goto-char (point-min))
     ,(let (marker-list)
	(with-temp-buffer
	  (insert (cond ((symbolp contents)
                         (symbol-value contents))
                        (t contents)))
	  (goto-char (point-min))
	  (while (re-search-forward elisp-test-point-marker-regex nil t)
	    (push (list (intern (match-string-no-properties 1))
			(match-beginning 0))
		  marker-list)
	    (delete-region (match-beginning 0)
			   (match-end 0))))
	`(let ,marker-list
	   ,@body))))
--8<---------------cut here---------------end--------------->8---

Here's how you can use it:

--8<---------------cut here---------------start------------->8---
(defvar mark-defun-test-buffer
  ";; Comment header
=!before-1=
\(defun func-1 (arg)
  =!inside-1=\"docstring\"
  body)
=!after-1==!before-2=
;; Comment before a defun
\(d=!inside-2=efun func-2 (arg)
  \"docstring\"
  body)
=!after-2==!before-3=
\(defun func-3 (arg)
  \"docstring\"=!inside-3=
  body)
=!after-3==!before-4=(defun func-4 (arg)
  \"docstring\"=!inside-4=
  body)
=!after-4=
;; end
"
  "Test buffer for `mark-defun'.")

(ert-deftest mark-defun-no-arg-region-inactive ()
  "Test `mark-defun' with no prefix argument and inactive
region."
  (setq last-command nil)
  (elisp-tests-with-temp-buffer
      mark-defun-test-buffer
    ;; mark-defun inside a defun, with comments and an empty line
    ;; before
    (goto-char inside-1)
    (mark-defun)
    (should (= (point) before-1))
    (should (= (mark) after-1))
    ;; mark-defun inside a defun with comments before
    (deactivate-mark)
    (goto-char inside-2)
    (mark-defun)
    (should (= (point) before-2))
    (should (= (mark) after-2))
    ;; mark-defun inside a defun with empty line before
    (deactivate-mark)
    (goto-char inside-3)
    (mark-defun)
    (should (= (point) before-3))
    (should (= (mark) after-3))
    ;; mark-defun inside a defun with another one right before
    (deactivate-mark)
    (goto-char inside-4)
    (mark-defun)
    (should (= (point) before-4))
    (should (= (mark) after-4))
    ;; mark-defun between a comment and a defun
    (deactivate-mark)
    (goto-char before-1)
    (mark-defun)
    (should (= (point) before-1))
    (should (= (mark) after-1))
    ;; mark-defun between defuns
    (deactivate-mark)
    (goto-char before-3)
    (mark-defun)
    (should (= (point) before-3))
    (should (= (mark) after-3))
    ;; mark-defun in comment right before the defun
    (deactivate-mark)
    (goto-char before-2)
    (mark-defun)
    (should (= (point) before-2))
    (should (= (mark) after-2))))
--8<---------------cut here---------------end--------------->8---

WDYT?

--
Marcin Borkowski

Re: Brave new mark-defun (and a testing tool)

[John Wiegley]

>>>>> "MB" == Marcin Borkowski <mbork@mbork.pl> writes:

MB> after several months of on-and-off work on bug#21072, I have implemented
MB> two (hopefully) nice features.  One is an (almost) completely new
MB> version of mark-defun, which I hope works much better than the previous
MB> one:

Can you clarify in what ways it is better? Reading through the text you
attached did not make it obvious to me...

Thanks,
-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-12, at 08:09, John Wiegley <jwiegley@gmail.com> wrote:

>>>>>> "MB" == Marcin Borkowski <mbork@mbork.pl> writes:
>
> MB> after several months of on-and-off work on bug#21072, I have implemented
> MB> two (hopefully) nice features.  One is an (almost) completely new
> MB> version of mark-defun, which I hope works much better than the previous
> MB> one:
>
> Can you clarify in what ways it is better? Reading through the text you
> attached did not make it obvious to me...

Well, sorry for that - all details are in the quite extensive thread
about bug 21072.  Here's a short summary of what "my" mark-defun tries
to accomplish (I hope my memory serves me well here...) in case you have
better things to do than reading through tens of old messages;-).

0. In order to test mark-defun more easily, I introduced the
elisp-tests-with-temp-buffer macro.  It accepts a string (or
a string-valued variable) and runs the rest of its body in an Elisp
buffer with that very string, with the exception that you can put
special "markers" in that string (by default, of the form "=!name="),
these markers are deleted from the temp buffer, and variables called
"name" etc. are then bound to markers pointing at these positions.
I would very much like some experienced Elisp hackers to look at it -
I'm not sure it is entirely correct/elegant.

1. mark-defun is now extensively tested - it comes with a suite of about
two dozen ert tests.

2. Bug#21072 is fixed - mark-defun between defuns marks the following
defun, as its docstring and the manual say.

3. Both positive and negative arguments work correctly when the region
is inactive (i.e., the right number of defuns are marked, either after
or before the point).  Both Drew and me (in the mentioned discussion)
agreed that this is better than the allow-extend argument used now
(which is probably quite useless anyway).

4. When the region is active, mark-defun selects one more defun
(assuming that the region contains one or more defuns already - this is
not checked), or abs(N) more defuns with an argument.  With negative
argument, the direction of selecting defuns is reversed (for the sake of
following mark-defun commands).

> Thanks,

You're welcome,

--
Marcin Borkowski

Re: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

Hey, one more thing.

On 2017-02-12, at 11:10, Marcin Borkowski <mbork@mbork.pl> wrote:

> On 2017-02-12, at 08:09, John Wiegley <jwiegley@gmail.com> wrote:
>
>> Can you clarify in what ways it is better? Reading through the text you
>> attached did not make it obvious to me...

Here's a fragment from one of Drew's messages from the thread
I mentioned in my previous message.

--8<---------------cut here---------------start------------->8---
What might be better?

1. At least consistency wrt which defun gets selected, when
betweeen defuns.  The doc suggests a general rule (the next
defun), but that is not always respected.

2. Something consistent also wrt a comment before the defun
that will be selected.

3. It could be good for a numeric prefix arg to select that
many defuns.

4. It could be good for a negative prefix arg to select in
the opposite direction.  This is the main improvement I'd
like to see.  E.g. `M-- C-M-h' selects the previous defun;
`M-2 C-M-h' selects the two previous defuns.

Someone should play around and dream up something useful.

Wrt #2, I'm not sure what the best approach might be.
--8<---------------cut here---------------end--------------->8---

5. I decided to mark the comment together with the defun if there is no
empty line between the comment and the defun and leave it unmarked
otherwise.  I guess this is the most sensible approach I could think of.

Best,

--
Marcin Borkowski

Re: Brave new mark-defun (and a testing tool)

[John Wiegley]

>>>>> Marcin Borkowski <mbork@mbork.pl> writes:

> mark-defun is now extensively tested - it comes with a suite of about two
> dozen ert tests.

Well, you've sold me there. :)

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-12, at 22:29, John Wiegley <jwiegley@gmail.com> wrote:

>>>>>> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> mark-defun is now extensively tested - it comes with a suite of about two
>> dozen ert tests.
>
> Well, you've sold me there. :)

Well, don't make fun of a humble mathematician. :-)

I figured that if I'm going to heavily change something as fundamental
as mark-defun, I'd better be sure I didn't break anything.  (In fact, as
Drew pointed out, I /did/ break a few things - my earlier tests did not
cover enough cases.  Hopefully it's better now...)

Also, I only tested it in Elisp mode.  That's a potential problem...

Best,

-- 
Marcin Borkowski

Re: Brave new mark-defun (and a testing tool)

[Dmitry Gutov]

On 13.02.2017 13:00, Marcin Borkowski wrote:

>>> mark-defun is now extensively tested - it comes with a suite of about two
>>> dozen ert tests.
>>
>> Well, you've sold me there. :)
> 
> Well, don't make fun of a humble mathematician. :-)

I'd rather interpret John as being entirely serious. :) Tests are good.

> Also, I only tested it in Elisp mode.  That's a potential problem...

I'm not a frequent user of mark-defun, so I can miss edge cases, but I 
see the improvement. Thanks!

Tested in emacs-lisp-mode and ruby-mode.

Re: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-13, at 16:16, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 13.02.2017 13:00, Marcin Borkowski wrote:
>
>>>> mark-defun is now extensively tested - it comes with a suite of about two
>>>> dozen ert tests.
>>>
>>> Well, you've sold me there. :)
>>
>> Well, don't make fun of a humble mathematician. :-)
>
> I'd rather interpret John as being entirely serious. :) Tests are good.

I'm not sure either way.  That's why I put a smiley in my response, too;-).

>> Also, I only tested it in Elisp mode.  That's a potential problem...
>
> I'm not a frequent user of mark-defun, so I can miss edge cases, but I
> see the improvement. Thanks!
>
> Tested in emacs-lisp-mode and ruby-mode.

Thanks a lot.  Python might be especially problematic (see comments in
code for why).

Best,

--
Marcin Borkowski

Re: Brave new mark-defun (and a testing tool)

[John Wiegley]

>>>>> Dmitry Gutov <dgutov@yandex.ru> writes:

> I'd rather interpret John as being entirely serious. :) Tests are good.

Dmitry is quite right; any patch that comes with a battery of new tests is
already a huge plus in my book.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]


On 2017-02-13, at 20:00, John Wiegley <jwiegley@gmail.com> wrote:

>>>>>> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> I'd rather interpret John as being entirely serious. :) Tests are good.
>
> Dmitry is quite right; any patch that comes with a battery of new tests is
> already a huge plus in my book.

Thanks - as I said, I was a bit unsure;-).

Here's my proposed contribution, formatted as two patches.  The first
one introduces the testing machinery; the second one introduces
mark-defun and its tests.

WDYT?

-- 
Marcin Borkowski

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-elisp-tests-with-temp-buffer-a-new-testing-macro.patch --]
[-- Type: text/x-diff, Size: 2634 bytes --]

From 0723e6a7c51bf3924e393e713f3509160d1782a6 Mon Sep 17 00:00:00 2001
From: Marcin Borkowski <mbork@mbork.pl>
Date: Tue, 14 Feb 2017 11:30:36 +0100
Subject: [PATCH] Add elisp-tests-with-temp-buffer, a new testing macro

* test/lisp/progmodes/elisp-mode-tests.el
(elisp-test-point-marker-regex) New variable.
(elisp-tests-with-temp-buffer): New macro to help test functions
moving the point and/or mark.
---
 test/lisp/progmodes/elisp-mode-tests.el | 39 +++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/test/lisp/progmodes/elisp-mode-tests.el b/test/lisp/progmodes/elisp-mode-tests.el
index 93c428b2d2..a00f6b1b43 100644
--- a/test/lisp/progmodes/elisp-mode-tests.el
+++ b/test/lisp/progmodes/elisp-mode-tests.el
@@ -2,6 +2,7 @@
 
 ;; Copyright (C) 2015-2017 Free Software Foundation, Inc.
 
+;; Author: Marcin Borkowski <mbork@mbork.pl>
 ;; Author: Dmitry Gutov <dgutov@yandex.ru>
 ;; Author: Stephen Leake <stephen_leake@member.fsf.org>
 
@@ -672,5 +673,43 @@ xref-elisp-overloadable-separate-default
     (insert "?\\N{HEAVY CHECK MARK}")
     (should (equal (elisp--preceding-sexp) ?\N{HEAVY CHECK MARK}))))
 
+;;; Helpers
+
+(defvar elisp-test-point-marker-regex "=!\\([a-zA-Z0-9-]+\\)="
+  "A regexp matching placeholders for point position for
+`elisp-tests-with-temp-buffer'.")
+
+;; Copied and heavily modified from `python-tests-with-temp-buffer'
+(defmacro elisp-tests-with-temp-buffer (contents &rest body)
+  "Create an `emacs-lisp-mode' enabled temp buffer with CONTENTS.
+BODY is the code to be executed within the temp buffer.  Point is
+always located at the beginning of buffer.  Special markers of
+the form =!NAME= in CONTENTS are removed, and a for each one
+a variable called NAME is bound to the position of such
+a marker."
+  (declare (indent 1) (debug t))
+  `(with-temp-buffer
+     (emacs-lisp-mode)
+     (insert ,contents)
+     (goto-char (point-min))
+     (while (re-search-forward elisp-test-point-marker-regex nil t)
+       (delete-region (match-beginning 0)
+		      (match-end 0)))
+     (goto-char (point-min))
+     ,(let (marker-list)
+	(with-temp-buffer
+	  (insert (cond ((symbolp contents)
+                         (symbol-value contents))
+                        (t contents)))
+	  (goto-char (point-min))
+	  (while (re-search-forward elisp-test-point-marker-regex nil t)
+	    (push (list (intern (match-string-no-properties 1))
+			(match-beginning 0))
+		  marker-list)
+	    (delete-region (match-beginning 0)
+			   (match-end 0))))
+	`(let ,marker-list
+	   ,@body))))
+
 (provide 'elisp-mode-tests)
 ;;; elisp-mode-tests.el ends here
-- 
2.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Fix-bug-21072-and-rework-mark-defun.patch --]
[-- Type: text/x-diff, Size: 13840 bytes --]

From 962f0c653891a4faf2e8db638defbc8096f9d3f1 Mon Sep 17 00:00:00 2001
From: Marcin Borkowski <mbork@mbork.pl>
Date: Tue, 14 Feb 2017 11:37:08 +0100
Subject: [PATCH] Fix bug#21072 and rework `mark-defun'

* test/lisp/progmodes/elisp-mode-tests.el (mark-defun-test-buffer):
  New variable
(mark-defun-no-arg-region-inactive)
(mark-defun-no-arg-region-active)
(mark-defun-arg-region-active)
(mark-defun-pos-arg-region-inactive)
(mark-defun-neg-arg-region-inactive, mark-defun-bob): Add tests for
the new `mark-defun'

* lisp/emacs-lisp/lisp.el (in-comment-line-p): New function
(beginning-of-defun-comments): New function
(mark-defun): Fix bug#21072, also rewrite large parts of `mark-defun'
to accept a numerical prefix argument
---
 lisp/emacs-lisp/lisp.el                 | 130 ++++++++++++-----
 test/lisp/progmodes/elisp-mode-tests.el | 245 ++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 38 deletions(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 0172e3af26..664691e629 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -398,6 +398,32 @@ beginning-of-defun-raw
 	     (goto-char (if arg-+ve floor ceiling))
 	     nil))))))))
 
+(defun in-comment-line-p ()
+  "Return non-nil if the point is in a comment line."
+;; See http://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00141.html
+  (save-excursion
+    (forward-line 0)
+    (unless (looking-at "^\\s-*$")
+      (< (line-end-position)
+         (let ((ppss (syntax-ppss)))
+           (when (nth 4 ppss)
+             (goto-char (nth 8 ppss)))
+           (forward-comment (point-max))
+           (point))))))
+
+(defun beginning-of-defun-comments (&optional arg)
+  "Move to the beginning of ARGth defun, including comments."
+  (interactive "^p")
+  (unless arg (setq arg 1))
+  (beginning-of-defun arg)
+  (let (nbobp)
+    (while (progn
+             (setq nbobp (zerop (forward-line -1)))
+             (and (in-comment-line-p)
+                  nbobp)))
+    (when nbobp
+      (forward-line 1))))
+
 (defvar end-of-defun-function
   (lambda () (forward-sexp 1))
   "Function for `end-of-defun' to call.
@@ -478,48 +504,76 @@ end-of-defun
         (funcall end-of-defun-function)
 	(funcall skip)))))
 
-(defun mark-defun (&optional allow-extend)
+(defun mark-defun (&optional arg)
   "Put mark at end of this defun, point at beginning.
 The defun marked is the one that contains point or follows point.
+With positive ARG, mark this and that many next defuns; with negative
+ARG, change the direction of marking.
 
-Interactively, if this command is repeated
-or (in Transient Mark mode) if the mark is active,
-it marks the next defun after the ones already marked."
+If the mark is active, it marks the next or previous defun(s) after
+the one(s) already marked."
   (interactive "p")
-  (cond ((and allow-extend
-	      (or (and (eq last-command this-command) (mark t))
-		  (and transient-mark-mode mark-active)))
-	 (set-mark
-	  (save-excursion
-	    (goto-char (mark))
-	    (end-of-defun)
-	    (point))))
-	(t
-	 (let ((opoint (point))
-	       beg end)
-	   (push-mark opoint)
-	   ;; 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.
-	   (beginning-of-defun)
-	   (setq beg (point))
-	   (end-of-defun)
-	   (setq end (point))
-	   (while (looking-at "^\n")
-	     (forward-line 1))
-	   (if (> (point) opoint)
-	       (progn
-		 ;; We got the right defun.
-		 (push-mark beg nil t)
-		 (goto-char end)
-		 (exchange-point-and-mark))
-	     ;; beginning-of-defun moved back one defun
-	     ;; so we got the wrong one.
-	     (goto-char opoint)
-	     (end-of-defun)
-	     (push-mark (point) nil t)
-	     (beginning-of-defun))
-	   (re-search-backward "^\n" (- (point) 1) t)))))
+  (setq arg (or arg 1))
+  ;; There is no `mark-defun-back' function - see
+  ;; https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-11/msg00079.html
+  ;; for explanation
+  (when (eq last-command 'mark-defun-back)
+    (setq arg (- arg)))
+  (when (< arg 0)
+    (setq this-command 'mark-defun-back))
+  (cond ((use-region-p)
+         (if (>= arg 0)
+             (set-mark
+              (save-excursion
+                (goto-char (mark))
+                ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
+                (dotimes (_ignore arg)
+                  (end-of-defun))
+                (point)))
+           (beginning-of-defun-comments (- arg))))
+        (t
+         (let ((opoint (point))
+               beg end)
+           (push-mark opoint)
+           ;; 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.
+           (beginning-of-defun-comments)
+           (setq beg (point))
+           (end-of-defun)
+           (setq end (point))
+           (when (or (and (<= (point) opoint)
+                          (> arg 0))
+                     (= beg (point-min))) ; we were before the first defun!
+             ;; beginning-of-defun moved back one defun so we got the wrong
+             ;; one.  If ARG < 0, however, we actually want to go back.
+             (goto-char opoint)
+             (end-of-defun)
+             (setq end (point))
+             (beginning-of-defun-comments)
+             (setq beg (point)))
+           (goto-char beg)
+           (cond ((> arg 0)
+                  ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
+                  (dotimes (_ignore arg)
+                    (end-of-defun))
+                  (setq end (point))
+                  (push-mark end nil t)
+                  (goto-char beg))
+                 (t
+                  (goto-char beg)
+                  (unless (= arg -1)    ; beginning-of-defun behaves
+                                        ; strange with zero arg - see
+                                        ; https://lists.gnu.org/archive/html/bug-gnu-emacs/2017-02/msg00196.html
+                    (beginning-of-defun (1- (- arg))))
+                  (push-mark end nil t))))))
+  (let (nbobp)
+    (while (progn
+             (setq nbobp (zerop (forward-line -1)))
+             (and (looking-at "^\\s-*$")
+                  nbobp)))
+    (when nbobp
+      (forward-line 1))))
 
 (defvar narrow-to-defun-include-comments nil
   "If non-nil, `narrow-to-defun' will also show comments preceding the defun.")
diff --git a/test/lisp/progmodes/elisp-mode-tests.el b/test/lisp/progmodes/elisp-mode-tests.el
index a00f6b1b43..2366e337df 100644
--- a/test/lisp/progmodes/elisp-mode-tests.el
+++ b/test/lisp/progmodes/elisp-mode-tests.el
@@ -711,5 +711,250 @@ elisp-tests-with-temp-buffer
 	`(let ,marker-list
 	   ,@body))))
 
+;;; mark-defun
+
+(defvar mark-defun-test-buffer
+  ";; Comment header
+=!before-1=
+\(defun func-1 (arg)
+  =!inside-1=\"docstring\"
+  body)
+=!after-1==!before-2=
+;; Comment before a defun
+\(d=!inside-2=efun func-2 (arg)
+  \"docstring\"
+  body)
+=!after-2==!before-3=
+\(defun func-3 (arg)
+  \"docstring\"=!inside-3=
+  body)
+=!after-3==!before-4=(defun func-4 (arg)
+  \"docstring\"=!inside-4=
+  body)
+=!after-4=
+;; end
+"
+  "Test buffer for `mark-defun'.")
+
+(ert-deftest mark-defun-no-arg-region-inactive ()
+  "Test `mark-defun' with no prefix argument and inactive
+region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun inside a defun, with comments and an empty line
+    ;; before
+    (goto-char inside-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun inside a defun with comments before
+    (deactivate-mark)
+    (goto-char inside-2)
+    (mark-defun)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))
+    ;; mark-defun inside a defun with empty line before
+    (deactivate-mark)
+    (goto-char inside-3)
+    (mark-defun)
+    (should (= (point) before-3))
+    (should (= (mark) after-3))
+    ;; mark-defun inside a defun with another one right before
+    (deactivate-mark)
+    (goto-char inside-4)
+    (mark-defun)
+    (should (= (point) before-4))
+    (should (= (mark) after-4))
+    ;; mark-defun between a comment and a defun
+    (deactivate-mark)
+    (goto-char before-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun)
+    (should (= (point) before-3))
+    (should (= (mark) after-3))
+    ;; mark-defun in comment right before the defun
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))))
+
+(ert-deftest mark-defun-no-arg-region-active ()
+  "Test `mark-defun' with no prefix argument and active
+region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun when a defun is marked
+    (goto-char before-1)
+    (set-mark after-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun when two defuns are marked
+    (deactivate-mark)
+    (goto-char before-1)
+    (set-mark after-2)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-arg-region-active ()
+  "Test `mark-defun' with a prefix arg and active region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with positive arg when a defun is marked
+    (goto-char before-1)
+    (set-mark after-1)
+    (mark-defun 2)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))
+    ;; mark-defun with arg=-1 when a defun is marked
+    (goto-char before-2)
+    (set-mark after-2)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun with arg=-2 when a defun is marked
+    (goto-char before-3)
+    (set-mark after-3)
+    (mark-defun -2)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-pos-arg-region-inactive ()
+  "Test `mark-defun' with positive argument and inactive
+  region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with positive arg inside a defun
+    (goto-char inside-1)
+    (mark-defun 2)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun with positive arg between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun 2)
+    (should (= (point) before-3))
+    (should (= (mark) after-4))
+    ;; mark-defun with positive arg in a comment
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun 2)
+    (should (= (point) before-2))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-neg-arg-region-inactive ()
+  "Test `mark-defun' with negative argument and inactive
+  region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with arg=-1 inside a defun
+    (goto-char inside-1)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun with arg=-1 between defuns
+    (deactivate-mark)
+    (goto-char after-2)
+    (mark-defun -1)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))
+    ;; mark-defun with arg=-1 in a comment
+    ;; (this is probably not an optimal behavior...)
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun with arg=-2 inside a defun
+    (deactivate-mark)
+    (goto-char inside-4)
+    (mark-defun -2)
+    (should (= (point) before-3))
+    (should (= (mark) after-4))
+    ;; mark-defun with arg=-2 between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun -2)
+    (should (= (point) before-1))
+    (should (= (mark) after-2)))
+  (elisp-tests-with-temp-buffer         ; test case submitted by Drew Adams
+      "(defun a ()
+  nil)
+=!before-b=(defun b ()
+=!in-b=  nil)
+=!after-b=;;;;
+\(defun c ()
+  nil)
+"
+    (setq last-command nil)
+    (goto-char in-b)
+    (mark-defun -1)
+    (should (= (point) before-b))
+    (should (= (mark) after-b))))
+
+(ert-deftest mark-defun-bob ()
+  "Test `mark-defun' at the beginning of buffer."
+  ;; Bob, comment, newline, defun
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      ";; Comment at the bob
+=!before=
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, newline, comment, defun
+  (elisp-tests-with-temp-buffer
+      "=!before=
+;; Comment before the defun
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, comment, defun
+  (elisp-tests-with-temp-buffer
+      "=!before=;; Comment at the bob before the defun
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, newline, comment, newline, defun
+  (elisp-tests-with-temp-buffer
+      "
+;; Comment before the defun
+=!before=
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after))))
+
 (provide 'elisp-mode-tests)
 ;;; elisp-mode-tests.el ends here
-- 
2.11.0

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Dmitry Gutov]

On 14.02.2017 12:45, Marcin Borkowski wrote:
> +(defun in-comment-line-p ()

This needs a different name.

Something like beginning-of-defun--in-comment-line-p might be a good choice.

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-14, at 14:02, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 14.02.2017 12:45, Marcin Borkowski wrote:
>> +(defun in-comment-line-p ()
>
> This needs a different name.
>
> Something like beginning-of-defun--in-comment-line-p might be a good choice.

Why?  It seems to me that it may be of general use.

Best,

-- 
Marcin Borkowski

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Stefan Monnier]

> Why?  It seems to me that it may be of general use.

If you want it to be general, it'll have to be better defined.
What is a "comment line"?


        Stefan

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-14, at 20:25, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> Why?  It seems to me that it may be of general use.
>
> If you want it to be general, it'll have to be better defined.
> What is a "comment line"?

A line containing only a comment (possibly after whitespace).  I guess
the docstring could benefit from explaining this.

Thanks,

-- 
Marcin Borkowski

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Stefan Monnier]

>>> Why?  It seems to me that it may be of general use.
>> If you want it to be general, it'll have to be better defined.
>> What is a "comment line"?
> A line containing only a comment (possibly after whitespace).

Is a line (using C syntax) like:

    /* blablabla

considered as a "comment line"?
What about the likely next line:

    blablabla */

?
How about

    blablabla

on a line between the previous two (i.e. within a comment)?

Regardless of the answer you give above, I'm wondering in which kind of
circumstance we'd want to test if we're on "a line containing only
a comment".


        Stefan

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-15, at 08:56, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>>> Why?  It seems to me that it may be of general use.
>>> If you want it to be general, it'll have to be better defined.
>>> What is a "comment line"?
>> A line containing only a comment (possibly after whitespace).
>
> Is a line (using C syntax) like:
>
>     /* blablabla
>
> considered as a "comment line"?

Yes.

> What about the likely next line:
>
>     blablabla */

Yes.

>
> ?
> How about
>
>     blablabla
>
> on a line between the previous two (i.e. within a comment)?

Yes.

(However, I found a minor bug: an empty line, even between a line "/*"
and another with "*/" is _not_ considered a comment line by my
function.  I'll try to fix it.

> Regardless of the answer you give above, I'm wondering in which kind of
> circumstance we'd want to test if we're on "a line containing only
> a comment".

You will be surprised, then, that I actually did use a very similar
function in completely another circumstance: a command that counts
source lines of code in a region, and excludes lines containing only
whitespace, comments and docstrings.  (Never mind the discussion about
whether SLOC is meaningful in any sense;-).)

Best,

--
Marcin Borkowski

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Stefan Monnier]

>> Regardless of the answer you give above, I'm wondering in which kind of
>> circumstance we'd want to test if we're on "a line containing only
>> a comment".
> You will be surprised, then, that I actually did use a very similar
> function in completely another circumstance: a command that counts
> source lines of code in a region, and excludes lines containing only
> whitespace, comments and docstrings.  (Never mind the discussion about
> whether SLOC is meaningful in any sense;-).)

My point is that it's not very frequent to need this exact definition of
a "comment line" and that there are various other possible definitions
one might need in other circumstances.
So at the very least, the doc should clarify which definition of
"comment line" it uses.


        Stefan

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-02-15, at 20:27, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>>> Regardless of the answer you give above, I'm wondering in which kind of
>>> circumstance we'd want to test if we're on "a line containing only
>>> a comment".
>> You will be surprised, then, that I actually did use a very similar
>> function in completely another circumstance: a command that counts
>> source lines of code in a region, and excludes lines containing only
>> whitespace, comments and docstrings.  (Never mind the discussion about
>> whether SLOC is meaningful in any sense;-).)
>
> My point is that it's not very frequent to need this exact definition of
> a "comment line" and that there are various other possible definitions
> one might need in other circumstances.
> So at the very least, the doc should clarify which definition of
> "comment line" it uses.

Understood.  Do you have then any better idea for the name of this
function?  beginning-of-defun--incomment-line-p seems to specific,
in-comment-line-p _may_ be indeed too general.  (I'll make the docstring
more precise, of course.)

Thank you all for looking at the patch,

-- 
Marcin Borkowski

bug#21072: Brave new mark-defun (and a testing tool)

[Stefan Monnier]

> Understood.  Do you have then any better idea for the name of this
> function?  beginning-of-defun--incomment-line-p seems to specific,
> in-comment-line-p _may_ be indeed too general.

I'll let someone else decide if it deserves a "non-prefixed" name, but
as for the name after the potential prefix, I think focusing on
"comment" is the wrong idea.  Maybe `insignificant-line-p`?  Or `emptyish-line-p`?


        Stefan

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]


On 2017-02-16, at 14:22, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> Understood.  Do you have then any better idea for the name of this
>> function?  beginning-of-defun--incomment-line-p seems to specific,
>> in-comment-line-p _may_ be indeed too general.
>
> I'll let someone else decide if it deserves a "non-prefixed" name, but
> as for the name after the potential prefix, I think focusing on
> "comment" is the wrong idea.  Maybe `insignificant-line-p`?  Or `emptyish-line-p`?

OK, so I have renamed it and expanded the docstring.  I attach
a corrected patch (the second one, the first one is the same as before).

Is there anything else I can do before we may apply this patch and
consider bug#21072 fixed?

(Notice that three places could be still corrected: two when bug#24427
is fixed and possibly another one when the strange behavior of
(beginning-of-defun 0) is fixed - I will officially file a bug about it
later.  But these apparently will have to wait.)

Best,

--
Marcin Borkowski

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Fix-bug-21072-and-rework-mark-defun.patch --]
[-- Type: text/x-diff, Size: 13922 bytes --]

From 618217607d0bfc7ed8d4090afabea040088a0951 Mon Sep 17 00:00:00 2001
From: Marcin Borkowski <mbork@mbork.pl>
Date: Tue, 14 Feb 2017 11:37:08 +0100
Subject: [PATCH] Fix bug#21072 and rework `mark-defun'

* test/lisp/progmodes/elisp-mode-tests.el (mark-defun-test-buffer):
  New variable
(mark-defun-no-arg-region-inactive)
(mark-defun-no-arg-region-active)
(mark-defun-arg-region-active)
(mark-defun-pos-arg-region-inactive)
(mark-defun-neg-arg-region-inactive, mark-defun-bob): Add tests for
the new `mark-defun'

* lisp/emacs-lisp/lisp.el (in-emptyish-line-p): New function
(beginning-of-defun-comments): New function
(mark-defun): Fix bug#21072, also rewrite large parts of `mark-defun'
to accept a numerical prefix argument
---
 lisp/emacs-lisp/lisp.el                 | 132 ++++++++++++-----
 test/lisp/progmodes/elisp-mode-tests.el | 245 ++++++++++++++++++++++++++++++++
 2 files changed, 339 insertions(+), 38 deletions(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 0172e3af26..28b136eba4 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -398,6 +398,34 @@ beginning-of-defun-raw
 	     (goto-char (if arg-+ve floor ceiling))
 	     nil))))))))
 
+(defun in-emptyish-line-p ()
+  "Return non-nil if the point is in an \"emptyish\" line.
+This means a line that consists entirely of comments and/or
+whitespace."
+;; See http://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00141.html
+  (save-excursion
+    (forward-line 0)
+    (< (line-end-position)
+       (let ((ppss (syntax-ppss)))
+         (when (nth 4 ppss)
+           (goto-char (nth 8 ppss)))
+         (forward-comment (point-max))
+         (point)))))
+
+(defun beginning-of-defun-comments (&optional arg)
+  "Move to the beginning of ARGth defun, including comments."
+  (interactive "^p")
+  (unless arg (setq arg 1))
+  (beginning-of-defun arg)
+  (let (nbobp)
+    (while (progn
+             (setq nbobp (zerop (forward-line -1)))
+             (and (not (looking-at "^\\s-*$"))
+                  (in-emptyish-line-p)
+                  nbobp)))
+    (when nbobp
+      (forward-line 1))))
+
 (defvar end-of-defun-function
   (lambda () (forward-sexp 1))
   "Function for `end-of-defun' to call.
@@ -478,48 +506,76 @@ end-of-defun
         (funcall end-of-defun-function)
 	(funcall skip)))))
 
-(defun mark-defun (&optional allow-extend)
+(defun mark-defun (&optional arg)
   "Put mark at end of this defun, point at beginning.
 The defun marked is the one that contains point or follows point.
+With positive ARG, mark this and that many next defuns; with negative
+ARG, change the direction of marking.
 
-Interactively, if this command is repeated
-or (in Transient Mark mode) if the mark is active,
-it marks the next defun after the ones already marked."
+If the mark is active, it marks the next or previous defun(s) after
+the one(s) already marked."
   (interactive "p")
-  (cond ((and allow-extend
-	      (or (and (eq last-command this-command) (mark t))
-		  (and transient-mark-mode mark-active)))
-	 (set-mark
-	  (save-excursion
-	    (goto-char (mark))
-	    (end-of-defun)
-	    (point))))
-	(t
-	 (let ((opoint (point))
-	       beg end)
-	   (push-mark opoint)
-	   ;; 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.
-	   (beginning-of-defun)
-	   (setq beg (point))
-	   (end-of-defun)
-	   (setq end (point))
-	   (while (looking-at "^\n")
-	     (forward-line 1))
-	   (if (> (point) opoint)
-	       (progn
-		 ;; We got the right defun.
-		 (push-mark beg nil t)
-		 (goto-char end)
-		 (exchange-point-and-mark))
-	     ;; beginning-of-defun moved back one defun
-	     ;; so we got the wrong one.
-	     (goto-char opoint)
-	     (end-of-defun)
-	     (push-mark (point) nil t)
-	     (beginning-of-defun))
-	   (re-search-backward "^\n" (- (point) 1) t)))))
+  (setq arg (or arg 1))
+  ;; There is no `mark-defun-back' function - see
+  ;; https://lists.gnu.org/archive/html/bug-gnu-emacs/2016-11/msg00079.html
+  ;; for explanation
+  (when (eq last-command 'mark-defun-back)
+    (setq arg (- arg)))
+  (when (< arg 0)
+    (setq this-command 'mark-defun-back))
+  (cond ((use-region-p)
+         (if (>= arg 0)
+             (set-mark
+              (save-excursion
+                (goto-char (mark))
+                ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
+                (dotimes (_ignore arg)
+                  (end-of-defun))
+                (point)))
+           (beginning-of-defun-comments (- arg))))
+        (t
+         (let ((opoint (point))
+               beg end)
+           (push-mark opoint)
+           ;; 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.
+           (beginning-of-defun-comments)
+           (setq beg (point))
+           (end-of-defun)
+           (setq end (point))
+           (when (or (and (<= (point) opoint)
+                          (> arg 0))
+                     (= beg (point-min))) ; we were before the first defun!
+             ;; beginning-of-defun moved back one defun so we got the wrong
+             ;; one.  If ARG < 0, however, we actually want to go back.
+             (goto-char opoint)
+             (end-of-defun)
+             (setq end (point))
+             (beginning-of-defun-comments)
+             (setq beg (point)))
+           (goto-char beg)
+           (cond ((> arg 0)
+                  ;; change the dotimes below to (end-of-defun arg) once bug #24427 is fixed
+                  (dotimes (_ignore arg)
+                    (end-of-defun))
+                  (setq end (point))
+                  (push-mark end nil t)
+                  (goto-char beg))
+                 (t
+                  (goto-char beg)
+                  (unless (= arg -1)    ; beginning-of-defun behaves
+                                        ; strange with zero arg - see
+                                        ; https://lists.gnu.org/archive/html/bug-gnu-emacs/2017-02/msg00196.html
+                    (beginning-of-defun (1- (- arg))))
+                  (push-mark end nil t))))))
+  (let (nbobp)
+    (while (progn
+             (setq nbobp (zerop (forward-line -1)))
+             (and (looking-at "^\\s-*$")
+                  nbobp)))
+    (when nbobp
+      (forward-line 1))))
 
 (defvar narrow-to-defun-include-comments nil
   "If non-nil, `narrow-to-defun' will also show comments preceding the defun.")
diff --git a/test/lisp/progmodes/elisp-mode-tests.el b/test/lisp/progmodes/elisp-mode-tests.el
index a00f6b1b43..2366e337df 100644
--- a/test/lisp/progmodes/elisp-mode-tests.el
+++ b/test/lisp/progmodes/elisp-mode-tests.el
@@ -711,5 +711,250 @@ elisp-tests-with-temp-buffer
 	`(let ,marker-list
 	   ,@body))))
 
+;;; mark-defun
+
+(defvar mark-defun-test-buffer
+  ";; Comment header
+=!before-1=
+\(defun func-1 (arg)
+  =!inside-1=\"docstring\"
+  body)
+=!after-1==!before-2=
+;; Comment before a defun
+\(d=!inside-2=efun func-2 (arg)
+  \"docstring\"
+  body)
+=!after-2==!before-3=
+\(defun func-3 (arg)
+  \"docstring\"=!inside-3=
+  body)
+=!after-3==!before-4=(defun func-4 (arg)
+  \"docstring\"=!inside-4=
+  body)
+=!after-4=
+;; end
+"
+  "Test buffer for `mark-defun'.")
+
+(ert-deftest mark-defun-no-arg-region-inactive ()
+  "Test `mark-defun' with no prefix argument and inactive
+region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun inside a defun, with comments and an empty line
+    ;; before
+    (goto-char inside-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun inside a defun with comments before
+    (deactivate-mark)
+    (goto-char inside-2)
+    (mark-defun)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))
+    ;; mark-defun inside a defun with empty line before
+    (deactivate-mark)
+    (goto-char inside-3)
+    (mark-defun)
+    (should (= (point) before-3))
+    (should (= (mark) after-3))
+    ;; mark-defun inside a defun with another one right before
+    (deactivate-mark)
+    (goto-char inside-4)
+    (mark-defun)
+    (should (= (point) before-4))
+    (should (= (mark) after-4))
+    ;; mark-defun between a comment and a defun
+    (deactivate-mark)
+    (goto-char before-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun)
+    (should (= (point) before-3))
+    (should (= (mark) after-3))
+    ;; mark-defun in comment right before the defun
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))))
+
+(ert-deftest mark-defun-no-arg-region-active ()
+  "Test `mark-defun' with no prefix argument and active
+region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun when a defun is marked
+    (goto-char before-1)
+    (set-mark after-1)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun when two defuns are marked
+    (deactivate-mark)
+    (goto-char before-1)
+    (set-mark after-2)
+    (mark-defun)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-arg-region-active ()
+  "Test `mark-defun' with a prefix arg and active region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with positive arg when a defun is marked
+    (goto-char before-1)
+    (set-mark after-1)
+    (mark-defun 2)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))
+    ;; mark-defun with arg=-1 when a defun is marked
+    (goto-char before-2)
+    (set-mark after-2)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun with arg=-2 when a defun is marked
+    (goto-char before-3)
+    (set-mark after-3)
+    (mark-defun -2)
+    (should (= (point) before-1))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-pos-arg-region-inactive ()
+  "Test `mark-defun' with positive argument and inactive
+  region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with positive arg inside a defun
+    (goto-char inside-1)
+    (mark-defun 2)
+    (should (= (point) before-1))
+    (should (= (mark) after-2))
+    ;; mark-defun with positive arg between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun 2)
+    (should (= (point) before-3))
+    (should (= (mark) after-4))
+    ;; mark-defun with positive arg in a comment
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun 2)
+    (should (= (point) before-2))
+    (should (= (mark) after-3))))
+
+(ert-deftest mark-defun-neg-arg-region-inactive ()
+  "Test `mark-defun' with negative argument and inactive
+  region."
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      mark-defun-test-buffer
+    ;; mark-defun with arg=-1 inside a defun
+    (goto-char inside-1)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun with arg=-1 between defuns
+    (deactivate-mark)
+    (goto-char after-2)
+    (mark-defun -1)
+    (should (= (point) before-2))
+    (should (= (mark) after-2))
+    ;; mark-defun with arg=-1 in a comment
+    ;; (this is probably not an optimal behavior...)
+    (deactivate-mark)
+    (goto-char before-2)
+    (mark-defun -1)
+    (should (= (point) before-1))
+    (should (= (mark) after-1))
+    ;; mark-defun with arg=-2 inside a defun
+    (deactivate-mark)
+    (goto-char inside-4)
+    (mark-defun -2)
+    (should (= (point) before-3))
+    (should (= (mark) after-4))
+    ;; mark-defun with arg=-2 between defuns
+    (deactivate-mark)
+    (goto-char before-3)
+    (mark-defun -2)
+    (should (= (point) before-1))
+    (should (= (mark) after-2)))
+  (elisp-tests-with-temp-buffer         ; test case submitted by Drew Adams
+      "(defun a ()
+  nil)
+=!before-b=(defun b ()
+=!in-b=  nil)
+=!after-b=;;;;
+\(defun c ()
+  nil)
+"
+    (setq last-command nil)
+    (goto-char in-b)
+    (mark-defun -1)
+    (should (= (point) before-b))
+    (should (= (mark) after-b))))
+
+(ert-deftest mark-defun-bob ()
+  "Test `mark-defun' at the beginning of buffer."
+  ;; Bob, comment, newline, defun
+  (setq last-command nil)
+  (elisp-tests-with-temp-buffer
+      ";; Comment at the bob
+=!before=
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, newline, comment, defun
+  (elisp-tests-with-temp-buffer
+      "=!before=
+;; Comment before the defun
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, comment, defun
+  (elisp-tests-with-temp-buffer
+      "=!before=;; Comment at the bob before the defun
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after)))
+  ;; Bob, newline, comment, newline, defun
+  (elisp-tests-with-temp-buffer
+      "
+;; Comment before the defun
+=!before=
+\(defun func (arg)=!inside=
+  \"docstring\"
+  body)
+=!after="
+    (goto-char inside)
+    (mark-defun)
+    (should (= (point) before))
+    (should (= (mark) after))))
+
 (provide 'elisp-mode-tests)
 ;;; elisp-mode-tests.el ends here
-- 
2.11.1

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Eli Zaretskii]

> From: Marcin Borkowski <mbork@mbork.pl>
> Date: Fri, 17 Feb 2017 09:54:51 +0100
> Cc: 21072@debbugs.gnu.org, emacs-devel@gnu.org
> 
> OK, so I have renamed it and expanded the docstring.  I attach
> a corrected patch (the second one, the first one is the same as before).
> 
> Is there anything else I can do before we may apply this patch and
> consider bug#21072 fixed?

Thanks, this looks good, but please move the tests to lisp-tests.el,
to keep our conventions wrt test names.

Then this could go in.

bug#21072: Brave new mark-defun (and a testing tool)

[Dmitry Gutov]

On 17.02.2017 10:54, Marcin Borkowski wrote:
> +(defun in-emptyish-line-p ()

In case you were wondering, I'm still not sure this is a valuable 
addition to our public API.

But if Eli says it's okay, then it's probably okay.

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 7 Mar 2017 18:50:33 +0200
> Cc: 21072@debbugs.gnu.org, emacs-devel@gnu.org
> 
> On 17.02.2017 10:54, Marcin Borkowski wrote:
> > +(defun in-emptyish-line-p ()
> 
> In case you were wondering, I'm still not sure this is a valuable 
> addition to our public API.
> 
> But if Eli says it's okay, then it's probably okay.

I don't really have an opinion, but perhaps it would be better to make
it an internal function for now, indeed.

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-03-07, at 17:50, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 17.02.2017 10:54, Marcin Borkowski wrote:
>> +(defun in-emptyish-line-p ()
>
> In case you were wondering, I'm still not sure this is a valuable
> addition to our public API.

Well, I renamed it anyway.

Thanks,

-- 
Marcin Borkowski

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-03-07, at 17:53, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 7 Mar 2017 18:50:33 +0200
>> Cc: 21072@debbugs.gnu.org, emacs-devel@gnu.org
>> 
>> On 17.02.2017 10:54, Marcin Borkowski wrote:
>> > +(defun in-emptyish-line-p ()
>> 
>> In case you were wondering, I'm still not sure this is a valuable 
>> addition to our public API.
>> 
>> But if Eli says it's okay, then it's probably okay.
>
> I don't really have an opinion, but perhaps it would be better to make
> it an internal function for now, indeed.

Yep, that's what I did.

-- 
Marcin Borkowski

Re: bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-03-07, at 17:46, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Marcin Borkowski <mbork@mbork.pl>
>> Date: Fri, 17 Feb 2017 09:54:51 +0100
>> Cc: 21072@debbugs.gnu.org, emacs-devel@gnu.org
>> 
>> OK, so I have renamed it and expanded the docstring.  I attach
>> a corrected patch (the second one, the first one is the same as before).
>> 
>> Is there anything else I can do before we may apply this patch and
>> consider bug#21072 fixed?
>
> Thanks, this looks good, but please move the tests to lisp-tests.el,
> to keep our conventions wrt test names.
>
> Then this could go in.

I just pushed it to a branch, forgetting about this email.  I'll fix it
and report back.  For now, I deleted the branch I pushed; I'll pish it
again as soon as I correct this.

Thanks,

-- 
Marcin Borkowski

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

On 2017-03-29, at 08:34, Marcin Borkowski <mbork@mbork.pl> wrote:

> On 2017-03-07, at 17:46, Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Marcin Borkowski <mbork@mbork.pl>
>>> Date: Fri, 17 Feb 2017 09:54:51 +0100
>>> Cc: 21072@debbugs.gnu.org, emacs-devel@gnu.org
>>> 
>>> OK, so I have renamed it and expanded the docstring.  I attach
>>> a corrected patch (the second one, the first one is the same as before).
>>> 
>>> Is there anything else I can do before we may apply this patch and
>>> consider bug#21072 fixed?
>>
>> Thanks, this looks good, but please move the tests to lisp-tests.el,
>> to keep our conventions wrt test names.
>>
>> Then this could go in.
>
> I just pushed it to a branch, forgetting about this email.  I'll fix it
> and report back.  For now, I deleted the branch I pushed; I'll pish it
> again as soon as I correct this.

OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
ok and either merge it into master or tell me that I can do it?

Best,

-- 
Marcin Borkowski

Re: bug#21072: Brave new mark-defun (and a testing tool)

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

>
> OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
> ok and either merge it into master or tell me that I can do it?

>  \f
> +** New macro 'elisp-tests-with-temp-buffer'
> +which helps writing tests for functions that should change buffers in
> +specific ways or manipulate point or mark positions.
> +
> +---

I don't this should be documented in NEWS since the macro is being added
to a test file, so it's not part of Emacs' libraries.  Also, the format
of the NEWS entry is wrong in the same way as the next one (see below).

> +With a prefix argument, it marks that many defuns or extends the
> +region by the appropriate number of defuns.  With negative prefix
> +argument it marks defuns in the opposite direction and also changes
> +the direction of selecting for subsequent uses of @code{mark-defun}.

This doesn't say what exactly happens with zero as argument.  The code
seems to do something odd.  Perhaps it should just be a user-error
instead?  Or maybe just a nop.

> modified   etc/NEWS
> @@ -363,6 +363,15 @@ words where first character is upper rather than title case, e.g.,
>  "DŽungla" instead of "Džungla".
>  
>  \f
> +** New behavior of 'mark-defun' implemented
> +Prefix argument selects that many (or that many more) defuns.
> +Negative prefix arg flips the direction of selection.  Also,
> +'mark-defun' between defuns correctly selects N following defuns (or
> +-N previous for negative arguments).  Finally, comments preceding the
> +defun are selected unless they are separated from the defun by a blank
> +line.
> +
> ++++
> * Changes in Specialized Modes and Packages in Emacs 26.1
> 

This entry should go before the page separator, and the "+++" should go
on the line just above the entry, not after it.

> +(defun beginning-of-defun-comments (&optional arg)

> +  (let (nbobp)
> +    (while (progn
> +             (setq nbobp (zerop (forward-line -1)))
> +             (and (not (looking-at "^\\s-*$"))
> +                  (beginning-of-defun--in-emptyish-line-p)
> +                  nbobp)))
> +    (when nbobp
> +      (forward-line 1))))


The looking-at call is redundant, right?  Anyway, can't that all be
replaced by just

    (forward-comment (- (point)))
    (unless (bolp)
      (forward-line 1))

> +(defun mark-defun (&optional arg)

> +  (let (nbobp)
> +    (while (progn
> +             (setq nbobp (zerop (forward-line -1)))
> +             (and (looking-at "^\\s-*$")
> +                  nbobp)))
> +    (when nbobp
> +      (forward-line 1))))

I think this can be just

    (skip-chars-backward "[:space:]\n")
    (unless (bolp)
      (forward-line 1))

bug#21072: Brave new mark-defun (and a testing tool)

[Marcin Borkowski]

Hey,

and thanks for your feedback!

My answers to particular points are below.


On 2017-04-03, at 00:56, npostavs@users.sourceforge.net wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>>
>> OK, I pushed the branch "fix-bug-21072".  Can anyone confirm that it's
>> ok and either merge it into master or tell me that I can do it?
>
>>  \f
>> +** New macro 'elisp-tests-with-temp-buffer'
>> +which helps writing tests for functions that should change buffers in
>> +specific ways or manipulate point or mark positions.
>> +
>> +---
>
> I don't this should be documented in NEWS since the macro is being added
> to a test file, so it's not part of Emacs' libraries.  Also, the format
> of the NEWS entry is wrong in the same way as the next one (see below).

I deleted that from etc/NEWS.

>> +With a prefix argument, it marks that many defuns or extends the
>> +region by the appropriate number of defuns.  With negative prefix
>> +argument it marks defuns in the opposite direction and also changes
>> +the direction of selecting for subsequent uses of @code{mark-defun}.
>
> This doesn't say what exactly happens with zero as argument.  The code
> seems to do something odd.  Perhaps it should just be a user-error
> instead?  Or maybe just a nop.

Good catch.  I guess a no-op is fine.

>> modified   etc/NEWS
>> @@ -363,6 +363,15 @@ words where first character is upper rather than title case, e.g.,
>>  "DŽungla" instead of "Džungla".
>>
>>  \f
>> +** New behavior of 'mark-defun' implemented
>> +Prefix argument selects that many (or that many more) defuns.
>> +Negative prefix arg flips the direction of selection.  Also,
>> +'mark-defun' between defuns correctly selects N following defuns (or
>> +-N previous for negative arguments).  Finally, comments preceding the
>> +defun are selected unless they are separated from the defun by a blank
>> +line.
>> +
>> ++++
>> * Changes in Specialized Modes and Packages in Emacs 26.1
>>
>
> This entry should go before the page separator, and the "+++" should go
> on the line just above the entry, not after it.

That one I do not understand.  This means that "+++" goes essentially
_to the previous entry_, which doesn't seem to make sense (especially
when viewing NEWS folded, which I assume everyone does, right?).

>> +(defun beginning-of-defun-comments (&optional arg)
>
>> +  (let (nbobp)
>> +    (while (progn
>> +             (setq nbobp (zerop (forward-line -1)))
>> +             (and (not (looking-at "^\\s-*$"))
>> +                  (beginning-of-defun--in-emptyish-line-p)
>> +                  nbobp)))
>> +    (when nbobp
>> +      (forward-line 1))))
>
>
> The looking-at call is redundant, right?  Anyway, can't that all be

Hm.  Probably yes, although this seems to be not very well documented in
`forward-comment's docs.

> replaced by just
>
>     (forward-comment (- (point)))
>     (unless (bolp)
>       (forward-line 1))

My tests say no.  Consider these contents of a buffer:

--8<---------------cut here---------------start------------->8---
;; Comment at the bob

(defun func (arg)
  "docstring"
  body)
--8<---------------cut here---------------end--------------->8---

Put the point inside the defun and call mark-defun.  Your version marks
the comment at the beginning, mine doesn't.

>> +(defun mark-defun (&optional arg)
>
>> +  (let (nbobp)
>> +    (while (progn
>> +             (setq nbobp (zerop (forward-line -1)))
>> +             (and (looking-at "^\\s-*$")
>> +                  nbobp)))
>> +    (when nbobp
>> +      (forward-line 1))))
>
> I think this can be just
>
>     (skip-chars-backward "[:space:]\n")
>     (unless (bolp)
>       (forward-line 1))

This OTOH does pass my tests, though I guess it would be clearer to
replace (bolp) with (bobp) in the above code (if I understand correctly,
in this situation they should be equivalent).  WDYT?

Thanks a lot,

--
Marcin Borkowski