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

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)

[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.

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)

[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

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)

[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

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

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

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)

[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

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

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

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.

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.

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.

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

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

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

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

[Glenn Morris]

Marcin Borkowski wrote:

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

Nitpick: branch should have been called fix/bug-21072
(given lack of response to http://debbugs.gnu.org/25610, I can't blame you)

While I'm nitpicking, please don't cc emacs-devel on bug reports.

:)

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

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

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

[Marcin Borkowski]

On 2017-04-02, at 22:22, Glenn Morris <rgm@gnu.org> wrote:

> Marcin Borkowski wrote:
>
>> 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?
>
> Nitpick: branch should have been called fix/bug-21072
> (given lack of response to http://debbugs.gnu.org/25610, I can't blame you)

OK, I'll fix that.  Thanks.

> While I'm nitpicking, please don't cc emacs-devel on bug reports.
>
> :)

Well, I guess the CC was somehow justified in that I wanted to ask
people about the new features I introduced while fixing an old bug.

Best,

--
Marcin Borkowski

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

[Noam Postavsky]

On Fri, Apr 7, 2017 at 4:25 AM, Marcin Borkowski <mbork@mbork.pl> wrote:

>>> +** New behavior of 'mark-defun' implemented
[...]
>>> +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?).

Hmm, no, I wasn't really aware of folding (you mean via
`outline-hide-sublevels', right?).

I guess the convention wasn't made with folding in mind, but the "+++"
and "---" markup is just temporary anyway.

>
>>> +(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))
>
> My tests say no.

Oh, right, I thought it was doing backward-comment, but the difference
is that it stops at blank lines, thus the *non-redundant* looking-at
call.

I wonder if that's a sensible thing to do for languages that have
multiline comments though, e.g. Javascript:

/*

This function returns 0

*/
function foo () {
    return 0;
}

Although we might say that such comments should have "*" on the empty lines.

>
>>> +(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?

Yes, I believe they are equivalent. I guess using bobp would explain
better when this happens, though I feel bolp better explains why we're
doing forward-line. I don't think it matters very much either way, go
with whichever you like best.

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

[Marcin Borkowski]

On 2017-04-07, at 16:41, Noam Postavsky <npostavs@users.sourceforge.net> wrote:

>>> 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?).
>
> Hmm, no, I wasn't really aware of folding (you mean via
> `outline-hide-sublevels', right?).

Actually, via Org-mode.  Is there anyone who does that without Org-mode?
Out of curiosity: why???

> I guess the convention wasn't made with folding in mind, but the "+++"
> and "---" markup is just temporary anyway.

OK.  Anyway, I fixed that.

> [...]
>
> Oh, right, I thought it was doing backward-comment, but the difference
> is that it stops at blank lines, thus the *non-redundant* looking-at
> call.
>
> I wonder if that's a sensible thing to do for languages that have
> multiline comments though, e.g. Javascript:
>
> /*
>
> This function returns 0
>
> */
> function foo () {
>     return 0;
> }
>
> Although we might say that such comments should have "*" on the empty lines.

Definitely.  OTOH, what if they don't...?  I'm not sure how to detect
such a situation.  Any ideas?

Best,

-- 
Marcin Borkowski

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

[Drew Adams]

> > Hmm, no, I wasn't really aware of folding (you mean via
> > `outline-hide-sublevels', right?).
> 
> Actually, via Org-mode.  Is there anyone who does that
> without Org-mode?  Out of curiosity: why???

Yes, I do. ;-)

And so do you, if you use `C-h n' (`view-emacs-news') and
you sometimes hide sublevels there.

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

[Eli Zaretskii]

> Date: Tue, 18 Apr 2017 07:04:10 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 21072@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > > Hmm, no, I wasn't really aware of folding (you mean via
> > > `outline-hide-sublevels', right?).
> > 
> > Actually, via Org-mode.  Is there anyone who does that
> > without Org-mode?  Out of curiosity: why???
> 
> Yes, I do. ;-)

And so do I.

> And so do you, if you use `C-h n' (`view-emacs-news') and
> you sometimes hide sublevels there.

Right.

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

>> Oh, right, I thought it was doing backward-comment, but the difference
>> is that it stops at blank lines, thus the *non-redundant* looking-at
>> call.
>>
>> I wonder if that's a sensible thing to do for languages that have
>> multiline comments though, e.g. Javascript:
>>
>> /*
>>
>> This function returns 0
>>
>> */
>> function foo () {
>>     return 0;
>> }
>>
>> Although we might say that such comments should have "*" on the empty lines.
>
> Definitely.  OTOH, what if they don't...?  I'm not sure how to detect
> such a situation.  Any ideas?

    (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)
      (while (let ((pt (prog1 (point) (forward-line -1)))
                   (ppss (syntax-ppss)))
               (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
                     ((and (parse-partial-sexp
                            (point) (line-end-position) nil t ppss)
                           (not (bolp)) (eolp)))
                     (t (goto-char pt) nil)))))

However there will always be some comment style that doesn't work, e.g.

    // Some description followed by a blank.

    function name(arg) {

    }

Another option is to give up the comment marking, it seems a bit
complicated to implement and explain to users.

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

[John Mastro]

<npostavs@users.sourceforge.net> wrote:
>     (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)
>       (while (let ((pt (prog1 (point) (forward-line -1)))
>                    (ppss (syntax-ppss)))
>                (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
>                      ((and (parse-partial-sexp
>                             (point) (line-end-position) nil t ppss)
>                            (not (bolp)) (eolp)))
>                      (t (goto-char pt) nil)))))
>
> However there will always be some comment style that doesn't work, e.g.
>
>     // Some description followed by a blank.
>
>     function name(arg) {
>
>     }
>
> Another option is to give up the comment marking, it seems a bit
> complicated to implement and explain to users.

Would it help to lean on (forward-comment -1) more?

Something like this:

(defun beginning-of-defun-comments (&optional arg)
  (interactive "^p")
  (let ((arg (or arg 1))
        point)
    (beginning-of-defun arg)
    (setq point (point))
    (while (not (eq point (setq point (progn (forward-comment -1) (point))))))
    (skip-chars-forward "[:space:]\r\n")))

Having to `skip-chars-forward' at the end seems a bit awkward, but I
think it does work on the recently mentioned JavaScript examples.

        John





        John

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

[John Mastro]

John Mastro <john.b.mastro@gmail.com> wrote:
> Would it help to lean on (forward-comment -1) more?
>
> Something like this:
>
> (defun beginning-of-defun-comments (&optional arg)
>   (interactive "^p")
>   (let ((arg (or arg 1))
>         point)
>     (beginning-of-defun arg)
>     (setq point (point))
>     (while (not (eq point (setq point (progn (forward-comment -1) (point))))))
>     (skip-chars-forward "[:space:]\r\n")))
>
> Having to `skip-chars-forward' at the end seems a bit awkward, but I
> think it does work on the recently mentioned JavaScript examples.

I realized that `forward-comment' returns nil when something besides a
comment or whitespace was found, so my idea boils down to:

(defun beginning-of-defun-comments (&optional arg)
  (interactive "^p")
  (beginning-of-defun (or arg 1))
  (while (forward-comment -1))
  (skip-chars-forward "[:space:]\r\n"))

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

[Marcin Borkowski]

On 2017-04-20, at 02:47, John Mastro <john.b.mastro@gmail.com> wrote:

> I realized that `forward-comment' returns nil when something besides a
> comment or whitespace was found, so my idea boils down to:
>
> (defun beginning-of-defun-comments (&optional arg)
>   (interactive "^p")
>   (beginning-of-defun (or arg 1))
>   (while (forward-comment -1))
>   (skip-chars-forward "[:space:]\r\n"))

Hi John,

this definitely does not work - my `beginning-of-defun-comments' stops
at the first blank line, yours does not.

Thanks anyway,

-- 
Marcin Borkowski

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

[Marcin Borkowski]

On 2017-04-19, at 02:04, npostavs@users.sourceforge.net wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>>> Oh, right, I thought it was doing backward-comment, but the difference
>>> is that it stops at blank lines, thus the *non-redundant* looking-at
>>> call.
>>>
>>> I wonder if that's a sensible thing to do for languages that have
>>> multiline comments though, e.g. Javascript:
>>>
>>> /*
>>>
>>> This function returns 0
>>>
>>> */
>>> function foo () {
>>>     return 0;
>>> }
>>>
>>> Although we might say that such comments should have "*" on the empty lines.
>>
>> Definitely.  OTOH, what if they don't...?  I'm not sure how to detect
>> such a situation.  Any ideas?
>
>     (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)
>       (while (let ((pt (prog1 (point) (forward-line -1)))
>                    (ppss (syntax-ppss)))
>                (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
>                      ((and (parse-partial-sexp
>                             (point) (line-end-position) nil t ppss)
>                            (not (bolp)) (eolp)))
>                      (t (goto-char pt) nil)))))

Still not there - I tried first on Elisp, like this:

;; A comment
(defun ...)

and it left the point at the end of the "A comment" line instead of at
the beginning...

> However there will always be some comment style that doesn't work, e.g.
>
>     // Some description followed by a blank.
>
>     function name(arg) {
>
>     }
>
> Another option is to give up the comment marking, it seems a bit
> complicated to implement and explain to users.

I'm tempted to leave it is it is in my branch.  For one, I'm a bit tired
by all this and I'd like to move on; also, as you said above, there is
little hope to do it "100% correctly", and I guess my solution may be
good enough.  (I'm pretty sure it's better than the status quo, at
least.)

I wouldn't like to resign from marking comments; I think it is pretty
useful.

So I'm going to delete the branch with the wrong name and push another
one, with the fixes we discussed earlier.

Best,

--
Marcin Borkowski

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

[Marcin Borkowski]

Hi all,

as I mentioned a few minutes ago, I pushed the fix/bug-21072 branch with
a few fixes discussed in this thread.  If it is in good enoug shape to
merge into master, please tell me and I'll do it; if not, I'll fix what
should be fixed (commit messages?)

Best,

-- 
Marcin Borkowski

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

> On 2017-04-19, at 02:04, npostavs@users.sourceforge.net wrote:
>
>> Marcin Borkowski <mbork@mbork.pl> writes:
>>
>>>> Oh, right, I thought it was doing backward-comment, but the difference
>>>> is that it stops at blank lines, thus the *non-redundant* looking-at
>>>> call.
>>>>
>>>> I wonder if that's a sensible thing to do for languages that have
>>>> multiline comments though, e.g. Javascript:
>>>>
>>>> /*
>>>>
>>>> This function returns 0
>>>>
>>>> */
>>>> function foo () {
>>>>     return 0;
>>>> }
>>>>
>>>> Although we might say that such comments should have "*" on the empty lines.
>>>
>>> Definitely.  OTOH, what if they don't...?  I'm not sure how to detect
>>> such a situation.  Any ideas?
>>
>>     (defun beginning-of-defun-comments (&optional arg)
[...]
>
> Still not there - I tried first on Elisp, like this:
>
> ;; A comment
> (defun ...)
>
> and it left the point at the end of the "A comment" line instead of at
> the beginning...

Hmm, I don't get that, although I did miss an inf loop when the comment
is at beginning of buffer.  Here is a fixed version:

    (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)
      (while (let ((pt (point))
                   (ppss (and (zerop (forward-line -1)) (syntax-ppss))))
               (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
                     ((and ppss
                           (parse-partial-sexp
                            (point) (line-end-position) nil t ppss)
                           (not (bolp)) (eolp)))
                     (t (goto-char pt) nil)))))

>
> I'm tempted to leave it is it is in my branch.  For one, I'm a bit tired
> by all this and I'd like to move on; also, as you said above, there is
> little hope to do it "100% correctly", and I guess my solution may be
> good enough.  (I'm pretty sure it's better than the status quo, at
> least.)

Yeah, since there's no perfect answer, I think it's fine if you just go
with whichever version you like best.  If people disagree, we'll find
out in bug reports :)

> as I mentioned a few minutes ago, I pushed the fix/bug-21072 branch with
> a few fixes discussed in this thread.  If it is in good enoug shape to
> merge into master, please tell me and I'll do it; if not, I'll fix what
> should be fixed (commit messages?)

Looks good to me (apart from the commit messages).

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

[Marcin Borkowski]

On 2017-04-22, at 20:05, npostavs@users.sourceforge.net wrote:

>> Still not there - I tried first on Elisp, like this:
>>
>> ;; A comment
>> (defun ...)
>>
>> and it left the point at the end of the "A comment" line instead of at
>> the beginning...
>
> Hmm, I don't get that, although I did miss an inf loop when the comment
> is at beginning of buffer.  Here is a fixed version:
>
>     (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)
>       (while (let ((pt (point))
>                    (ppss (and (zerop (forward-line -1)) (syntax-ppss))))
>                (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
>                      ((and ppss
>                            (parse-partial-sexp
>                             (point) (line-end-position) nil t ppss)
>                            (not (bolp)) (eolp)))
>                      (t (goto-char pt) nil)))))

Still the same here, also in emacs -Q.

>> I'm tempted to leave it is it is in my branch.  For one, I'm a bit tired
>> by all this and I'd like to move on; also, as you said above, there is
>> little hope to do it "100% correctly", and I guess my solution may be
>> good enough.  (I'm pretty sure it's better than the status quo, at
>> least.)
>
> Yeah, since there's no perfect answer, I think it's fine if you just go
> with whichever version you like best.  If people disagree, we'll find
> out in bug reports :)

;-)

>> as I mentioned a few minutes ago, I pushed the fix/bug-21072 branch with
>> a few fixes discussed in this thread.  If it is in good enoug shape to
>> merge into master, please tell me and I'll do it; if not, I'll fix what
>> should be fixed (commit messages?)
>
> Looks good to me (apart from the commit messages).

What should they look like, then?  Should I make all of them into the
ChangeLog format, or just rebase/squash all of them so that there are
two of them only (one for the testing tool and one for the
beginning-of-defun)?

Best,

-- 
Marcin Borkowski

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

> On 2017-04-22, at 20:05, npostavs@users.sourceforge.net wrote:
>
>>> Still not there - I tried first on Elisp, like this:
>>>
>>> ;; A comment
>>> (defun ...)
>>>
>>> and it left the point at the end of the "A comment" line instead of at
>>> the beginning...
>>
>> Hmm, I don't get that, although I did miss an inf loop when the comment
>> is at beginning of buffer.  Here is a fixed version:
>
> Still the same here, also in emacs -Q.

Hmm, I was testing before now just by evaluating the changed functions
after startup, I must have messed something up.  I think this one might
be okay.

    (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)
      (while (let ((pt (line-beginning-position))
                   (ppss (and (zerop (forward-line -1)) (syntax-ppss))))
               (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
                     ((and ppss
                           (parse-partial-sexp
                            (point) (line-end-position) nil t ppss)
                           (not (bolp)) (eolp)))
                     (t (goto-char pt) nil)))))

>>
>> Looks good to me (apart from the commit messages).

Actually, now that I've applied this thing properly, I notice 2 test
failures in batch mode:

2 unexpected results:
   FAILED  mark-defun-arg-region-active
   FAILED  mark-defun-no-arg-region-active

They pass when run in interactive mode (I haven't looked into why).

>
> What should they look like, then?  Should I make all of them into the
> ChangeLog format, or just rebase/squash all of them so that there are
> two of them only (one for the testing tool and one for the
> beginning-of-defun)?

Yeah, I would go ahead and squash them, since you need to rebase to fix
commit messages anyway.

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

[Marcin Borkowski]

On 2017-04-24, at 14:52, npostavs@users.sourceforge.net wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> On 2017-04-22, at 20:05, npostavs@users.sourceforge.net wrote:
>>
>>>> Still not there - I tried first on Elisp, like this:
>>>>
>>>> ;; A comment
>>>> (defun ...)
>>>>
>>>> and it left the point at the end of the "A comment" line instead of at
>>>> the beginning...
>>>
>>> Hmm, I don't get that, although I did miss an inf loop when the comment
>>> is at beginning of buffer.  Here is a fixed version:
>>
>> Still the same here, also in emacs -Q.
>
> Hmm, I was testing before now just by evaluating the changed functions
> after startup, I must have messed something up.  I think this one might
> be okay.
>
>     (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)
>       (while (let ((pt (line-beginning-position))
>                    (ppss (and (zerop (forward-line -1)) (syntax-ppss))))
>                (cond ((nth 4 ppss) (goto-char (nth 8 ppss)))
>                      ((and ppss
>                            (parse-partial-sexp
>                             (point) (line-end-position) nil t ppss)
>                            (not (bolp)) (eolp)))
>                      (t (goto-char pt) nil)))))

Quick tests show that you are right.  I'd like to understand the code,
too - that will take me a while.

>>> Looks good to me (apart from the commit messages).
>
> Actually, now that I've applied this thing properly, I notice 2 test
> failures in batch mode:
>
> 2 unexpected results:
>    FAILED  mark-defun-arg-region-active
>    FAILED  mark-defun-no-arg-region-active
>
> They pass when run in interactive mode (I haven't looked into why).

That seems strange - all tests pass here with M-x ert.  What do you mean
by "batch mode"?

>> What should they look like, then?  Should I make all of them into the
>> ChangeLog format, or just rebase/squash all of them so that there are
>> two of them only (one for the testing tool and one for the
>> beginning-of-defun)?
>
> Yeah, I would go ahead and squash them, since you need to rebase to fix
> commit messages anyway.

Will do.

Thanks,

-- 
Marcin Borkowski

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

>>
>> Actually, now that I've applied this thing properly, I notice 2 test
>> failures in batch mode:
>>
>> 2 unexpected results:
>>    FAILED  mark-defun-arg-region-active
>>    FAILED  mark-defun-no-arg-region-active
>>
>> They pass when run in interactive mode (I haven't looked into why).
>
> That seems strange - all tests pass here with M-x ert.

Yes, same here (that's what I mean by "interactive mode").

> What do you mean by "batch mode"?

I mean 'emacs -Q -batch -l .../lisp-tests.el -f ert-run-tests-batch-and-exit'
which is what 'make check' or 'cd test && make lisp-tests' do.

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

[Noam Postavsky]

On Tue, Apr 25, 2017 at 7:43 AM, Marcin Borkowski <mbork@mbork.pl> wrote:
>>
>> Hmm, I was testing before now just by evaluating the changed functions
>> after startup, I must have messed something up.  I think this one might
>> be okay.
>
> Quick tests show that you are right.  I'd like to understand the code,
> too - that will take me a while.

I started commenting the code a bit which made me think of some more
strange corner cases, like

/* foo */ foo;
function () {
    return 0;
}

and I ended up with this

(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 (bobp)
    (while (let ((ppss (progn (setq bobp (= (forward-line -1) -1))
                              (syntax-ppss (line-end-position)))))
             (while (and (nth 4 ppss) ; If eol is in a line-spanning comment,
                         (< (nth 8 ppss) (line-beginning-position)))
               (goto-char (nth 8 ppss)) ; skip to comment start.
               (setq ppss (syntax-ppss (line-end-position))))
             (and (not bobp)
                  (progn (skip-syntax-backward
                          "-" (line-beginning-position))
                         (not (bolp))) ; Check for blank line.
                  (progn (parse-partial-sexp
                          (line-beginning-position) (line-end-position)
                          nil t (syntax-ppss (line-beginning-position)))
                         (eolp))))) ; Check for non-comment text.
    (forward-line (if bobp 0 1))))

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

[Marcin Borkowski]

On 2017-04-24, at 14:52, npostavs@users.sourceforge.net wrote:

> Actually, now that I've applied this thing properly, I notice 2 test
> failures in batch mode:
>
> 2 unexpected results:
>    FAILED  mark-defun-arg-region-active
>    FAILED  mark-defun-no-arg-region-active
>
> They pass when run in interactive mode (I haven't looked into why).

Strangely enough, they also fail with my original code.  Does anyone
have any idea why that might be the case?

Best,

-- 
Marcin Borkowski

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

[Noam Postavsky]

On Thu, Apr 27, 2017 at 12:43 PM, Marcin Borkowski <mbork@mbork.pl> wrote:
>
> On 2017-04-24, at 14:52, npostavs@users.sourceforge.net wrote:
>
>> Actually, now that I've applied this thing properly, I notice 2 test
>> failures in batch mode:
>>
>> 2 unexpected results:
>>    FAILED  mark-defun-arg-region-active
>>    FAILED  mark-defun-no-arg-region-active
>>
>> They pass when run in interactive mode (I haven't looked into why).
>
> Strangely enough, they also fail with my original code.  Does anyone
> have any idea why that might be the case?

Oh I see what it is, your test depends on `transient-mark-mode' being
enabled. With --batch, that defaults to nil. Should we be testing in
both modes or just explicitly set it to t?

PS I noticed some problems with elisp-tests-with-temp-buffer while
looking at this, I pushed a fix to fix/bug-21072 [1: 04741f0].

1: 2017-04-27 17:38:09 -0400 04741f02cca95147581e05cb49b54e6dbf8bed56
  Fix elisp-tests-with-temp-buffer compilation

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

[Marcin Borkowski]

On 2017-04-27, at 23:48, Noam Postavsky <npostavs@users.sourceforge.net> wrote:

> On Thu, Apr 27, 2017 at 12:43 PM, Marcin Borkowski <mbork@mbork.pl> wrote:
>>
>> On 2017-04-24, at 14:52, npostavs@users.sourceforge.net wrote:
>>
>>> Actually, now that I've applied this thing properly, I notice 2 test
>>> failures in batch mode:
>>>
>>> 2 unexpected results:
>>>    FAILED  mark-defun-arg-region-active
>>>    FAILED  mark-defun-no-arg-region-active
>>>
>>> They pass when run in interactive mode (I haven't looked into why).
>>
>> Strangely enough, they also fail with my original code.  Does anyone
>> have any idea why that might be the case?
>
> Oh I see what it is, your test depends on `transient-mark-mode' being
> enabled. With --batch, that defaults to nil. Should we be testing in
> both modes or just explicitly set it to t?

Good catch, thanks!  I'll set it to t, since this functionality is
intended to be used in t-t-m anyway.  OTOH, I'm curious where it breaks
with that disabled - I'll look into it now, I guess.

> PS I noticed some problems with elisp-tests-with-temp-buffer while
> looking at this, I pushed a fix to fix/bug-21072 [1: 04741f0].
>
> 1: 2017-04-27 17:38:09 -0400 04741f02cca95147581e05cb49b54e6dbf8bed56
>   Fix elisp-tests-with-temp-buffer compilation

Thanks a lot, I'll take a look!

Best,

-- 
Marcin Borkowski

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

[Marcin Borkowski]

On 2017-04-27, at 23:48, Noam Postavsky <npostavs@users.sourceforge.net> wrote:

> PS I noticed some problems with elisp-tests-with-temp-buffer while
> looking at this, I pushed a fix to fix/bug-21072 [1: 04741f0].
>
> 1: 2017-04-27 17:38:09 -0400 04741f02cca95147581e05cb49b54e6dbf8bed56
>   Fix elisp-tests-with-temp-buffer compilation

I started looking at this, and I have a few questions.

1. What is the difference between (debug t) and (debug form body)?  From
my understanding of the manual, there seems to be none.

2. I still don't fully get the `eval-and-compile' stuff.  What exactly
happens when the file is compiled?  I understand that its forms are not
evaluated.  But are the _defmacro_ forms evaluated during compilation?
If not, what happens when the compiler compiles a function containing
a macro invocation?

3. Do I get it correctly that the purpose of the `ignore' calls is to
silence the compiler warnings about `let'ting a variable which is then
not used in the body of `let'?

That's it for now.  Sorry if my questions are elementary - I'm still
struggling with this compilation stuff...

TIA,

-- 
Marcin Borkowski

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

[Stefan Monnier]

> 1. What is the difference between (debug t) and (debug form body)?  From
> my understanding of the manual, there seems to be none.

Indeed, no significant difference.

> 2. I still don't fully get the `eval-and-compile' stuff.  What exactly
> happens when the file is compiled?

During compilation, (eval-and-compile E) will be treated (i.e. compiled)
just like E, except that E is evaluated along the way (so the
definitions present in E are available during compilation).

> I understand that its forms are not evaluated.

They are.  It's for `eval-when-compile` that they aren't.

> But are the _defmacro_ forms evaluated during compilation?

The defmacro forms are always evaluated (and byte-compiled) during
compilation (as if they were implicitly wrapped inside an
eval-and-compile).

> 3. Do I get it correctly that the purpose of the `ignore' calls is to
> silence the compiler warnings about `let'ting a variable which is then
> not used in the body of `let'?

Yes.


        Stfean

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

[Noam Postavsky]

On Sun, Apr 30, 2017 at 12:10 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> 1. What is the difference between (debug t) and (debug form body)?  From
>> my understanding of the manual, there seems to be none.
>
> Indeed, no significant difference.

Oops, I meant (debug sexp body), then I changed it for some reason and
forgot to change back to the correct thing.

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

[Stefan Monnier]

>>> 1. What is the difference between (debug t) and (debug form body)?  From
>>> my understanding of the manual, there seems to be none.
>> Indeed, no significant difference.
> Oops, I meant (debug sexp body), then I changed it for some reason and
> forgot to change back to the correct thing.

Ah that changes everything: (debug (sexp body)) is quite different from
(debug t) since it says that the first argument is not an expression
(and hence shouldn't be instrumented).


        Stefan

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

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> 1. What is the difference between (debug t) and (debug form body)?  From
>>>> my understanding of the manual, there seems to be none.
>>> Indeed, no significant difference.
>> Oops, I meant (debug sexp body), then I changed it for some reason and
>> forgot to change back to the correct thing.
>
> Ah that changes everything: (debug (sexp body)) is quite different from
> (debug t) since it says that the first argument is not an expression
> (and hence shouldn't be instrumented).

I guess it's related to the fact that the first argument is evaluated by
the macro itself at compile time, with (debug t) or (debug form body) I
get an error when trying to instrument one of tests using that macro:

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p (edebug-after 0 3 mark-defun-test-buffer))
  insert((edebug-after 0 3 mark-defun-test-buffer))
  (progn (insert (cond ((symbolp contents) (symbol-value contents)) (t contents))) (goto-char (point-min)) [...])
  (unwind-protect [...] (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))
  (save-current-buffer (set-buffer temp-buffer) [...])
  (let ((temp-buffer (generate-new-buffer " *temp*"))) [...])
  (let* ((var-pos nil) (text (let ((temp-buffer (generate-new-buffer " *temp*"))) [...]))) [...])
  (closure (t) (contents &rest body) "Create an `emacs-lisp-mode' enabled temp buffer with CONTENTS[...]
  macroexpand((elisp-tests-with-temp-buffer [...]
  [...]
  edebug-eval-defun((4))
  apply(edebug-eval-defun (4))
  eval-defun((4))
  funcall-interactively(eval-defun (4))
  call-interactively(eval-defun nil nil)
  command-execute(eval-defun)

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

[Stefan Monnier]

> I guess it's related to the fact that the first argument is evaluated by
> the macro itself at compile time, with (debug t) or (debug form body) I
> get an error when trying to instrument one of tests using that macro:

Yes, you can either use (debug (sexp body)) to avoid instrumenting the
first arg, or you might use (debug (def-form body)) so as to warn that
the first argument is not evaluated at the "normal" time.


        Stefan


> Debugger entered--Lisp error: (wrong-type-argument char-or-string-p
> (edebug-after 0 3 mark-defun-test-buffer))
>   insert((edebug-after 0 3 mark-defun-test-buffer))
>   (progn (insert (cond ((symbolp contents) (symbol-value contents)) (t
> contents))) (goto-char (point-min)) [...])
>   (unwind-protect [...] (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))
>   (save-current-buffer (set-buffer temp-buffer) [...])
>   (let ((temp-buffer (generate-new-buffer " *temp*"))) [...])
>   (let* ((var-pos nil) (text (let ((temp-buffer (generate-new-buffer " *temp*"))) [...]))) [...])
>   (closure (t) (contents &rest body) "Create an `emacs-lisp-mode' enabled
> temp buffer with CONTENTS[...]
>   macroexpand((elisp-tests-with-temp-buffer [...]
>   [...]
>   edebug-eval-defun((4))
>   apply(edebug-eval-defun (4))
>   eval-defun((4))
>   funcall-interactively(eval-defun (4))
>   call-interactively(eval-defun nil nil)
>   command-execute(eval-defun)

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

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I guess it's related to the fact that the first argument is evaluated by
>> the macro itself at compile time, with (debug t) or (debug form body) I
>> get an error when trying to instrument one of tests using that macro:
>
> Yes, you can either use (debug (sexp body)) to avoid instrumenting the
> first arg, or you might use (debug (def-form body)) so as to warn that
> the first argument is not evaluated at the "normal" time.

I'm not sure exactly what def-form means, but using it gives a similar
error.

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p (edebug-enter (quote test@mark-defun-no-arg-region-inactive) nil (function (lambda nil (edebug-after 0 3 mark-defun-test-buffer)))))
  insert((edebug-enter (quote test@mark-defun-no-arg-region-inactive) nil (function (lambda nil (edebug-after 0 3 mark-defun-test-buffer)))))
  [...]

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

[Stefan Monnier]

>>> I guess it's related to the fact that the first argument is evaluated by
>>> the macro itself at compile time, with (debug t) or (debug form body) I
>>> get an error when trying to instrument one of tests using that macro:
>> 
>> Yes, you can either use (debug (sexp body)) to avoid instrumenting the
>> first arg, or you might use (debug (def-form body)) so as to warn that
>> the first argument is not evaluated at the "normal" time.

> I'm not sure exactly what def-form means, but using it gives a similar
> error.

> Debugger entered--Lisp error: (wrong-type-argument char-or-string-p
> (edebug-enter (quote test@mark-defun-no-arg-region-inactive) nil (function
> (lambda nil (edebug-after 0 3 mark-defun-test-buffer)))))
>   insert((edebug-enter (quote test@mark-defun-no-arg-region-inactive) nil
> (function (lambda nil (edebug-after 0 3 mark-defun-test-buffer)))))
>   [...]

This suggests that the first arg is not "evaluated by the macro itself
at compile time", but instead is passed unevaluated to `insert`, in
which case it can't be instrumented.


        Stefan

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

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This suggests that the first arg is not "evaluated by the macro itself
> at compile time", but instead is passed unevaluated to `insert`, in
> which case it can't be instrumented.

Oh, I see, this patch works, although it surprised me a bit to start
stepping immediately when doing the C-u C-M-x.

---   i/test/lisp/emacs-lisp/lisp-tests.el
+++   w/test/lisp/emacs-lisp/lisp-tests.el
@@ -320,12 +320,10 @@ elisp-tests-with-temp-buffer
 of the form =!NAME= in CONTENTS are removed, and a for each one a
 variable called NAME is bound to the position of the word's
 start."
-  (declare (indent 1) (debug (form body)))
+  (declare (indent 1) (debug (def-form body)))
   (let* ((var-pos nil)
          (text (with-temp-buffer
-                 (insert (cond ((symbolp contents)
-                                (symbol-value contents))
-                               (t contents)))
+                 (insert (eval contents))
                  (goto-char (point-min))
                  (while (re-search-forward elisp-test-point-position-regex nil t)
                    (push (list (intern (match-string-no-properties 1))

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

[Marcin Borkowski]

On 2017-04-30, at 18:10, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> 2. I still don't fully get the `eval-and-compile' stuff.  What exactly
>> happens when the file is compiled?
>
> During compilation, (eval-and-compile E) will be treated (i.e. compiled)
> just like E, except that E is evaluated along the way (so the
> definitions present in E are available during compilation).
>
>> I understand that its forms are not evaluated.
>
> They are.  It's for `eval-when-compile` that they aren't.

Thanks for your answer, but I'm still not sure.  Are the tables below
right?

Running:

| form                            | evaled? | compiled? |
|---------------------------------+---------+-----------|
| (defun ...)                     | yes     | no        |
| (eval-when-compile (defun ...)) | no      | no        |
| (eval-and-compile (defun ...))  | yes     | no        |
| (defmacro ...)                  | yes     | no        |

Compilation:

| form                            | evaled? | compiled? |
|---------------------------------+---------+-----------|
| (defun ...)                     | no      | yes       |
| (eval-when-compile (defun ...)) | yes     | yes       |
| (eval-and-compile (defun ...))  | yes     | yes       |
| (defmacro ...)                  | yes     | yes       |

TIA,

--
Marcin Borkowski

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

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

> Thanks for your answer, but I'm still not sure.  Are the tables below
> right?
>
> Running:
>
> | form                            | evaled? | compiled? |
> |---------------------------------+---------+-----------|
> | (defun ...)                     | yes     | no        |
> | (eval-when-compile (defun ...)) | no      | no        |
> | (eval-and-compile (defun ...))  | yes     | no        |
> | (defmacro ...)                  | yes     | no        |

I find this table very confusing.  Does the second column really mean
"Is the form evaluated when it is compiled when it is not compiled"?

I suggest to have a look at the definitions.  AFAICT the semantics of
`eval-when-compile' and `eval-and-compile' are defined by (1) their
regular definitions in byte-run.el and (2) the specifications in the
definition of `byte-compile-initial-macro-environment'.  Even if you
don't know much details, it all should be human readable.


Regards,

Michael.

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

[Stefan Monnier]

> Running:

I assume you mean "without compilation" (i.e. using only the .el file).

> | form                            | evaled? | compiled? |
> |---------------------------------+---------+-----------|
> | (defun ...)                     | yes     | no        |
> | (eval-when-compile (defun ...)) | no      | no        |
                                      ^^
                                      yes

> Compilation:

And 

> | form                            | evaled? | compiled? |
> |---------------------------------+---------+-----------|
> | (defun ...)                     | no      | yes       |
> | (eval-when-compile (defun ...)) | yes     | yes       |
                                                ^^^
                                                no

Instead, the whole expression is compiled as a constant holding the
value returned from the evaluation.  Hence this can be used for things
like (eval-when-compile (+ 3 4)).


        Stefan

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

[Marcin Borkowski]

On 2017-05-01, at 00:21, npostavs@users.sourceforge.net wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> This suggests that the first arg is not "evaluated by the macro itself
>> at compile time", but instead is passed unevaluated to `insert`, in
>> which case it can't be instrumented.
>
> Oh, I see, this patch works, although it surprised me a bit to start
> stepping immediately when doing the C-u C-M-x.
>
> ---   i/test/lisp/emacs-lisp/lisp-tests.el
> +++   w/test/lisp/emacs-lisp/lisp-tests.el
> @@ -320,12 +320,10 @@ elisp-tests-with-temp-buffer
>  of the form =!NAME= in CONTENTS are removed, and a for each one a
>  variable called NAME is bound to the position of the word's
>  start."
> -  (declare (indent 1) (debug (form body)))
> +  (declare (indent 1) (debug (def-form body)))
>    (let* ((var-pos nil)
>           (text (with-temp-buffer
> -                 (insert (cond ((symbolp contents)
> -                                (symbol-value contents))
> -                               (t contents)))
> +                 (insert (eval contents))
>                   (goto-char (point-min))
>                   (while (re-search-forward elisp-test-point-position-regex nil t)
>                     (push (list (intern (match-string-no-properties 1))

Hi Stefan and Noam,

I finally analyzed Noam's code, and the very same thing occurred to me:
that using `(eval contents)' would both be more reasonable (after all,
CONTENTS might be neither a symbol nor a string, but some expression!)
and at the same time consistent with Noam's rewording of the docstring.
Thanks!

I'm in a train now, and with limited access to the internet, so I'll get
back to this thread (and push the branch after clean-up - I guess I'll
have to delete the current branch first, right?) soon.

It's good that the Emacs manuals are available offline - I'll read about
arguments to `debug' now. ;-)

Best,

--
Marcin Borkowski

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

[Marcin Borkowski]

Hi Noam, Stefan et al.,

I finally found some time to make the branch nicer, deleted it from the
server and pushed the new one.  Could you look at it one more time and
merge it to master (or greenlight me to do it)?

Best,

--
Marcin Borkowski

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

> I finally found some time to make the branch nicer, deleted it from the
> server and pushed the new one.  Could you look at it one more time and
> merge it to master (or greenlight me to do it)?

[[c1c3403cf2]]
> 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)

There should be a colon after the close paren on each of those lines.

> (mark-defun-neg-arg-region-inactive, mark-defun-bob): Add tests for
> the new `mark-defun'
>
> * lisp/emacs-lisp/lisp.el (beginning-of-defun--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

There should be a period at the end of each entry.

[[aeed45da]]
> * lisp/emacs-lisp/lisp.el (mark-defun): simplify moving the point

Missing capitalization and period (if you're not just squashing this).


[[b8fd2c2ca1]]
> * Fix elisp-tests-with-temp-buffer compilation
[...]

Please add to this the patch from
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21072#254.

[[7efd2f2ea1]]
> Modify `beginning-of-defun-comments'

If you are keeping this one, I would add to the commit message:

* lisp/emacs-lisp/lisp.el (beginning-of-defun-comments): Try not to stop
in the middle of a multiline comment.

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

[Stefan Monnier]

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

> There should be a colon after the close paren on each of those lines.

Actually, AFAIK the normal "many functions" format looks like:

	* filename (functionname1, functionname2)
	(functionname3, functionname4)
	(functionname5, functionname6)
	(functionname7, functionname8): New functions.

we even tried to make the auto-fill code DTRT in the change-log-mode.


        Stefan

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

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Actually, AFAIK the normal "many functions" format looks like:
>
> 	* filename (functionname1, functionname2)
> 	(functionname3, functionname4)
> 	(functionname5, functionname6)
> 	(functionname7, functionname8): New functions.
>
> we even tried to make the auto-fill code DTRT in the change-log-mode.

Oh, huh, I took the example in CONTRIBUTE (under "Preferred form for
several entries with the same content:") to mean that lines should end
with colon, but I guess that only applies to multiple file entries, not
functions?  The GNU ChangeLog standards do indeed say to end the line
with just a close paren [1].

[1]: https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html

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

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Tue, 09 May 2017 23:31:24 -0400
> Cc: Marcin Borkowski <mbork@mbork.pl>, 21072@debbugs.gnu.org
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > Actually, AFAIK the normal "many functions" format looks like:
> >
> > 	* filename (functionname1, functionname2)
> > 	(functionname3, functionname4)
> > 	(functionname5, functionname6)
> > 	(functionname7, functionname8): New functions.
> >
> > we even tried to make the auto-fill code DTRT in the change-log-mode.
> 
> Oh, huh, I took the example in CONTRIBUTE (under "Preferred form for
> several entries with the same content:") to mean that lines should end
> with colon, but I guess that only applies to multiple file entries, not
> functions?

Yes, correct.

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

[Marcin Borkowski]

On 2017-05-10, at 04:53, npostavs@users.sourceforge.net wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> I finally found some time to make the branch nicer, deleted it from the
>> server and pushed the new one.  Could you look at it one more time and
>> merge it to master (or greenlight me to do it)?
>
> [[c1c3403cf2]]
>> 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)
>
> There should be a colon after the close paren on each of those lines.
>
>> (mark-defun-neg-arg-region-inactive, mark-defun-bob): Add tests for
>> the new `mark-defun'
>>
>> * lisp/emacs-lisp/lisp.el (beginning-of-defun--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
>
> There should be a period at the end of each entry.
>
> [[aeed45da]]
>> * lisp/emacs-lisp/lisp.el (mark-defun): simplify moving the point
>
> Missing capitalization and period (if you're not just squashing this).
>
>
> [[b8fd2c2ca1]]
>> * Fix elisp-tests-with-temp-buffer compilation
> [...]
>
> Please add to this the patch from
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21072#254.
>
> [[7efd2f2ea1]]
>> Modify `beginning-of-defun-comments'
>
> If you are keeping this one, I would add to the commit message:
>
> * lisp/emacs-lisp/lisp.el (beginning-of-defun-comments): Try not to stop
> in the middle of a multiline comment.

Thanks.

All done, branch deleted and pushed again.  Hopefully it's ok now.  (I'm
still learning to write commit messages...)

Best,

-- 
Marcin Borkowski

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

[npostavs]

Marcin Borkowski <mbork@mbork.pl> writes:

> All done, branch deleted and pushed again.  Hopefully it's ok now.

Looks good to me.

> (I'm still learning to write commit messages...)

Turns out, so am I :)

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

[Marcin Borkowski]

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

> All done, branch deleted and pushed again.  Hopefully it's ok now.  (I'm
> still learning to write commit messages...)

Hi all,

since seemingly nobody objected, I merged my branch into master and
marked the bug as done.

Best,

-- 
Marcin Borkowski

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

[Glenn Morris]

It seems this causes two python tests to fail.
See eg http://hydra.nixos.org/build/52829789

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

[npostavs]

Glenn Morris <rgm@gnu.org> writes:

> It seems this causes two python tests to fail.
> See eg http://hydra.nixos.org/build/52829789

It seems again to be an issue of different behaviour with
transient-mark-mode disabled.

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

[npostavs]

npostavs@users.sourceforge.net writes:

> Glenn Morris <rgm@gnu.org> writes:
>
>> It seems this causes two python tests to fail.
>> See eg http://hydra.nixos.org/build/52829789
>
> It seems again to be an issue of different behaviour with
> transient-mark-mode disabled.

I've set the tests to use transient-mark-mode [1: ee54d2f4e4], we might
have to revisit the non transient-mark-mode behaviour though.

[1: ee54d2f4e4]: 2017-05-20 18:28:23 -0400
  ; Set transient-mark-mode to let mark-defun tests pass
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ee54d2f4e439b4a211c8fb7541ce22bac65bde8f