Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Stefan Monnier]

> +@defopt edebug-after-instrumentation-functions
> +An abnormal hook run by Edebug after it instruments a form.
> +Each function is called with one argument, a form which has
> +just been instrumented by Edebug.

This seems a bit awkward.

How 'bout something like the 100% untested patch below?


        Stefan


diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index a070ff25d1..cfdd699bd9 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1072,9 +1072,9 @@ edebug-after-instrumentation-functions
 Each function is called with one argument, a form which has just
 been instrumented for Edebugging.")
 
-(defvar edebug-new-definition-functions '(edebug-announce-definition)
-  "Abnormal hook run after Edebug wraps a new definition.
-After Edebug has initialized its own data, each hook function is
+(defvar edebug-new-definition-function #'edebug-announce-definition
+  "Function to call after Edebug wraps a new definition.
+After Edebug has initialized its own data, this function is
 called with one argument, the symbol associated with the
 definition, which may be the actual symbol defined or one
 generated by Edebug.")
@@ -1383,13 +1383,13 @@ edebug-make-form-wrapper
 		 edebug-offset-list
 		 edebug-top-window-data
 		 ))
-      (put edebug-def-name 'edebug-behavior 'edebug)
-      (run-hook-with-args 'edebug-new-definition-functions edebug-def-name)
+      (funcall edebug-new-definition-function edebug-def-name)
       result
       )))
 
 (defun edebug-announce-definition (def-name)
   "Announce Edebug's processing of DEF-NAME."
+  (put def-name 'edebug-behavior 'edebug)
   (message "Edebug: %s" def-name))
 
 
diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index 3628968974..be07bc82cb 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -195,10 +195,10 @@ testcover-start
           testcover-module-potentially-1value-functions nil)
     (cl-letf ((edebug-all-defs t)
               (edebug-after-instrumentation-functions)
-              (edebug-new-definition-functions))
+              (edebug-new-definition-function))
       (add-hook 'edebug-after-instrumentation-functions 'testcover-after-instrumentation)
-      (add-hook 'edebug-new-definition-functions 'testcover-init-definition)
-      (remove-hook 'edebug-new-definition-functions 'edebug-announce-definition)
+      (add-function :override edebug-new-definition-function
+                    #'testcover-init-definition)
       (eval-buffer buf)))
   (when byte-compile
     (dolist (x (reverse edebug-form-data))
@@ -212,10 +212,10 @@ testcover-this-defun
   (interactive)
   (cl-letf ((edebug-all-defs t)
             (edebug-after-instrumentation-functions)
-            (edebug-new-definition-functions))
+            (edebug-new-definition-function))
     (add-hook 'edebug-after-instrumentation-functions 'testcover-after-instrumentation)
-    (add-hook 'edebug-new-definition-functions 'testcover-init-definition)
-    (remove-hook 'edebug-new-definition-functions 'edebug-announce-definition)
+    (add-function :override edebug-new-definition-function
+                  #'testcover-init-definition)
     (eval-defun nil)))
 
 (defun testcover-end (filename)

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Gemini Lasswell]

Stefan Monnier writes:

> This seems a bit awkward.
>
> How 'bout something like the 100% untested patch below?

OK with me. I'll test the patch and make the change. Do you think
edebug-after-instrumentation-functions should also be changed to
edebug-after-instrumentation-function?

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Stefan Monnier]

>> This seems a bit awkward.
>> How 'bout something like the 100% untested patch below?
> OK with me. I'll test the patch and make the change. Do you think
> edebug-after-instrumentation-functions should also be changed to
> edebug-after-instrumentation-function?

I guess we could use a patch like below, indeed, which would make it
possible to change the instrumented code (i.e. it would be slightly
more general).

It's not as important, tho, because
edebug-after-instrumentation-functions starts as nil, so it works OK,
contrarily to edebug-new-definition-functions where we need to
remove-hook its default value, which is really ugly.


        Stefan



diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index a070ff25d1..3381543f0a 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1067,8 +1067,8 @@ edebug-best-error
 
 ;; Hooks which may be used to extend Edebug's functionality.  See
 ;; Testcover for an example.
-(defvar edebug-after-instrumentation-functions nil
-  "Abnormal hook run on code after instrumentation for debugging.
+(defvar edebug-after-instrumentation-function #'identity
+  "FIXME Abnormal hook run on code after instrumentation for debugging.
 Each function is called with one argument, a form which has just
 been instrumented for Edebugging.")
 
@@ -1189,8 +1189,7 @@ edebug-read-and-maybe-wrap-form1
 
             ;; Not a defining form, and not edebugging.
             (t (edebug-read-sexp)))))
-      (run-hook-with-args 'edebug-after-instrumentation-functions result)
-      result)))
+      (funcall edebug-after-instrumentation-function result))))
 
 (defvar edebug-def-args) ; args of defining form.
 (defvar edebug-def-interactive) ; is it an emacs interactive function?

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Alan Mackenzie]

Hello, Stefan.

On Mon, Oct 09, 2017 at 00:13:24 -0400, Stefan Monnier wrote:
> > +@defopt edebug-after-instrumentation-functions
> > +An abnormal hook run by Edebug after it instruments a form.
> > +Each function is called with one argument, a form which has
> > +just been instrumented by Edebug.

> This seems a bit awkward.

Why?  How is it at all awkward?

> How 'bout something like the 100% untested patch below?

You don't say why you want to make the patch.  It looks to me like you
want to replace a normal abnormal hook with a single function.  This is
less flexible (unless one jumps through hoops).  It is also
incompatible; it will break applications which use the existing
interface.

Why do you want to break these applications?  What benefits will the
new single function scheme bring?

I'm against this incompatible change (which I don't fully understand).

>         Stefan

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Stefan Monnier]

> You don't say why you want to make the patch.

No, indeed.  I think the patch speaks for itself, and the author of the
original code seems to agree.

> It looks to me like you want to replace a normal abnormal hook with
> a single function.  This is less flexible (unless one jumps through
> hoops).

Look at the patch again: the resulting code is simpler.  And if you
want, you can reimplement the previous
edebug-after-instrumentation-functions API on top of the new, because
it's actually *more* flexible.

> It is also incompatible; it will break applications which use the
> existing interface.

Yes, it changes a hook which was introduced what 3 days ago?
I promise I'll take care of the all backlash from all the previous users.

> I'm against this incompatible change

Duly noted.

> (which I don't fully understand).

I had noticed, yes ;-)


        Stefan

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Alan Mackenzie]

Hello, Stefan.

On Wed, Oct 11, 2017 at 16:32:53 -0400, Stefan Monnier wrote:
> > You don't say why you want to make the patch.

> No, indeed.  I think the patch speaks for itself, and the author of the
> original code seems to agree.

> > It looks to me like you want to replace a normal abnormal hook with
> > a single function.  This is less flexible (unless one jumps through
> > hoops).

> Look at the patch again: the resulting code is simpler.  And if you
> want, you can reimplement the previous
> edebug-after-instrumentation-functions API on top of the new, because
> it's actually *more* flexible.

> > It is also incompatible; it will break applications which use the
> > existing interface.

> Yes, it changes a hook which was introduced what 3 days ago?
> I promise I'll take care of the all backlash from all the previous users.

Ah, OK.  I got the impression this was a traditional hook.

> > I'm against this incompatible change

> Duly noted.

> > (which I don't fully understand).

> I had noticed, yes ;-)

Now that I understand it better, I withdraw my objection.  Thanks for
the explanation!

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Gemini Lasswell]

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


Stefan Monnier writes:

>>> This seems a bit awkward.
>>> How 'bout something like the 100% untested patch below?
>> OK with me. I'll test the patch and make the change. Do you think
>> edebug-after-instrumentation-functions should also be changed to
>> edebug-after-instrumentation-function?
>
> I guess we could use a patch like below, indeed, which would make it
> possible to change the instrumented code (i.e. it would be slightly
> more general).

Here's a tested patch, with documentation. In it I've changed both
hooks to variables, and chosen to let-bind them in Testcover. I don't
know why you used add-function in your first patch, but if you can
educate me, I'll change what needs changing.


[-- Attachment #2: 0001-Change-Edebug-s-behavior-changing-hooks-to-variables.patch --]
[-- Type: text/plain, Size: 9261 bytes --]

From b5b44de3b7a24f98a02f7aee4fe12ca961508c7f Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Sat, 14 Oct 2017 09:13:36 -0700
Subject: [PATCH] Change Edebug's behavior-changing hooks to variables

* lisp/emacs-lisp/edebug.el (edebug-after-instrumentation-functions)
(edebug-new-definition-functions): Deleted.
(edebug-after-instrumentation-function)
(edebug-new-definition-function): New variables.
(edebug-behavior-alist): Update docstring.
(edebug-read-and-maybe-wrap-form1, edebug-make-form-wrapper): Use new
variables.

* lisp/emacs-lisp/testcover.el (testcover-start)
(testcover-this-defun): Use `edebug-after-instrumentation-function' and
`edebug-new-definition-function'.
(testcover-after-instrumentation): Return passed form.
(testcover-init-definition): Use argument instead of `edebug-def-name'.

* doc/lispref/edebug.texi (Edebug Options): Replace descriptions of
`edebug-after-instrumentation-functions' and `edebug-new-definition-functions'
with `edebug-after-instrumentation-function' and
`edebug-new-definition-function'.
---
 doc/lispref/edebug.texi      | 23 +++++++++--------------
 etc/NEWS                     |  6 +++---
 lisp/emacs-lisp/edebug.el    | 37 +++++++++++++++++++------------------
 lisp/emacs-lisp/testcover.el | 23 +++++++++--------------
 4 files changed, 40 insertions(+), 49 deletions(-)

diff --git a/doc/lispref/edebug.texi b/doc/lispref/edebug.texi
index 94d61480f1..651bfacb4c 100644
--- a/doc/lispref/edebug.texi
+++ b/doc/lispref/edebug.texi
@@ -1705,23 +1705,18 @@ Edebug Options
 call the new functions in place of its own for that definition.
 @end defopt
 
-@defopt edebug-new-definition-functions
-An abnormal hook run by Edebug after it wraps the body of a definition
-or closure.  After Edebug has initialized its own data, each function
+@defopt edebug-new-definition-function
+A function run by Edebug after it wraps the body of a definition
+or closure.  After Edebug has initialized its own data, this function
 is called with one argument, the symbol associated with the
 definition, which may be the actual symbol defined or one generated by
-Edebug. This hook may be used to set the @code{edebug-behavior}
+Edebug.  This function may be used to set the @code{edebug-behavior}
 symbol property of each definition instrumented by Edebug.
-
-By default @code{edebug-new-definition-functions} contains
-@code{edebug-announce-definition} which prints a message each time a
-definition is instrumented.  If you are instrumenting a lot of code
-and find the messages excessive, remove
-@code{edebug-announce-definition}.
 @end defopt
 
-@defopt edebug-after-instrumentation-functions
-An abnormal hook run by Edebug after it instruments a form.
-Each function is called with one argument, a form which has
-just been instrumented by Edebug.
+@defopt edebug-after-instrumentation-function
+To inspect or modify Edebug's instrumentation before it is used, set
+this variable to a function which takes one argument, an instrumented
+top-level form, and returns either the same or a replacement form,
+which Edebug will then use as the final result of instrumentation.
 @end defopt
diff --git a/etc/NEWS b/etc/NEWS
index 2332ba4d1f..809f8e87fc 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -60,9 +60,9 @@ replaced by a double typographic quote.
 
 +++
 *** The runtime behavior of Edebug's instrumentation can be changed
-using the new variable 'edebug-behavior-alist' and the new abnormal
-hooks 'edebug-after-instrumentation-functions' and
-'edebug-new-definition-functions'. Edebug's behavior can be changed
+using the new variables 'edebug-behavior-alist',
+'edebug-after-instrumentation-function' and
+'edebug-new-definition-function'. Edebug's behavior can be changed
 globally or for individual definitions.
 
 ** Enhanced xterm support
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 77523de32c..0e8f77e29a 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1065,16 +1065,17 @@ edebug-old-def-name
 (defvar edebug-error-point nil)
 (defvar edebug-best-error nil)
 
-;; Hooks which may be used to extend Edebug's functionality.  See
+;; Functions which may be used to extend Edebug's functionality.  See
 ;; Testcover for an example.
-(defvar edebug-after-instrumentation-functions nil
-  "Abnormal hook run on code after instrumentation for debugging.
-Each function is called with one argument, a form which has just
-been instrumented for Edebugging.")
-
-(defvar edebug-new-definition-functions '(edebug-announce-definition)
-  "Abnormal hook run after Edebug wraps a new definition.
-After Edebug has initialized its own data, each hook function is
+(defvar edebug-after-instrumentation-function #'identity
+  "Function to run on code after instrumentation for debugging.
+The function is called with one argument, a FORM which has just
+been instrumented for Edebugging, and it should return either FORM
+or a replacement form to use in its place.")
+
+(defvar edebug-new-definition-function #'edebug-new-definition
+  "Function to call after Edebug wraps a new definition.
+After Edebug has initialized its own data, this function is
 called with one argument, the symbol associated with the
 definition, which may be the actual symbol defined or one
 generated by Edebug.")
@@ -1087,9 +1088,9 @@ edebug-behavior-alist
 the instrumented code is running, Edebug will look here for the
 implementations of `edebug-enter', `edebug-before', and
 `edebug-after'.  Edebug's instrumentation may be used for a new
-purpose by adding an entry to this alist and a hook to
-`edebug-new-definition-functions' which sets `edebug-behavior'
-for the definition.")
+purpose by adding an entry to this alist, and setting
+`edebug-new-definition-function' to a function which sets
+`edebug-behavior' for the definition.")
 
 (defun edebug-read-and-maybe-wrap-form ()
   ;; Read a form and wrap it with edebug calls, if the conditions are right.
@@ -1189,8 +1190,7 @@ edebug-read-and-maybe-wrap-form1
 
             ;; Not a defining form, and not edebugging.
             (t (edebug-read-sexp)))))
-      (run-hook-with-args 'edebug-after-instrumentation-functions result)
-      result)))
+      (funcall edebug-after-instrumentation-function result))))
 
 (defvar edebug-def-args) ; args of defining form.
 (defvar edebug-def-interactive) ; is it an emacs interactive function?
@@ -1383,13 +1383,14 @@ edebug-make-form-wrapper
 		 edebug-offset-list
 		 edebug-top-window-data
 		 ))
-      (put edebug-def-name 'edebug-behavior 'edebug)
-      (run-hook-with-args 'edebug-new-definition-functions edebug-def-name)
+
+      (funcall edebug-new-definition-function edebug-def-name)
       result
       )))
 
-(defun edebug-announce-definition (def-name)
-  "Announce Edebug's processing of DEF-NAME."
+(defun edebug-new-definition (def-name)
+  "Set up DEF-NAME to use Edebug's instrumentation functions."
+  (put def-name 'edebug-behavior 'edebug)
   (message "Edebug: %s" def-name))
 
 
diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index 3628968974..797cc68217 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -193,12 +193,9 @@ testcover-start
           testcover-module-constants nil
           testcover-module-1value-functions nil
           testcover-module-potentially-1value-functions nil)
-    (cl-letf ((edebug-all-defs t)
-              (edebug-after-instrumentation-functions)
-              (edebug-new-definition-functions))
-      (add-hook 'edebug-after-instrumentation-functions 'testcover-after-instrumentation)
-      (add-hook 'edebug-new-definition-functions 'testcover-init-definition)
-      (remove-hook 'edebug-new-definition-functions 'edebug-announce-definition)
+    (let ((edebug-all-defs t)
+          (edebug-after-instrumentation-function #'testcover-after-instrumentation)
+          (edebug-new-definition-function #'testcover-init-definition))
       (eval-buffer buf)))
   (when byte-compile
     (dolist (x (reverse edebug-form-data))
@@ -210,12 +207,9 @@ testcover-start
 (defun testcover-this-defun ()
   "Start coverage on function under point."
   (interactive)
-  (cl-letf ((edebug-all-defs t)
-            (edebug-after-instrumentation-functions)
-            (edebug-new-definition-functions))
-    (add-hook 'edebug-after-instrumentation-functions 'testcover-after-instrumentation)
-    (add-hook 'edebug-new-definition-functions 'testcover-init-definition)
-    (remove-hook 'edebug-new-definition-functions 'edebug-announce-definition)
+  (let ((edebug-all-defs t)
+        (edebug-after-instrumentation-function #'testcover-after-instrumentation)
+        (edebug-new-definition-function #'testcover-init-definition))
     (eval-defun nil)))
 
 (defun testcover-end (filename)
@@ -231,11 +225,12 @@ testcover-end
 
 (defun testcover-after-instrumentation (form)
   "Analyze FORM for code coverage."
-  (testcover-analyze-coverage form))
+  (testcover-analyze-coverage form)
+  form)
 
 (defun testcover-init-definition (sym)
   "Mark SYM as under test coverage."
-  (message "Testcover: %s" edebug-def-name)
+  (message "Testcover: %s" sym)
   (put sym 'edebug-behavior 'testcover))
 
 (defun testcover-enter (func _args body)
-- 
2.14.2

Re: [Emacs-diffs] master 06e452a 1/3: Allow Edebug's instrumentation to be used for other purposes

[Stefan Monnier]

> Here's a tested patch, with documentation. In it I've changed both
> hooks to variables, and chosen to let-bind them in Testcover.

Looks great.

> I don't know why you used add-function in your first patch, but if you can
> educate me, I'll change what needs changing.

I think it was just to better match the previous code which used add-hook.
Your solution using just let seems cleaner,


        Stefan