bug#25316: Patch for bug#25316: 26.0.50; Bugs in testcover-reinstrument

[Eli Zaretskii]

> From: Gemini Lasswell <gazally@runbox.com>
> Date: Mon, 25 Sep 2017 15:04:58 -0700
> 
> The attached patches fix the 5 bugs in this bug report, as well as
> 11307, 24509, 24688, 24743 and 25326.
> 
> testcover-reinstrument is a code walker which rewrites Edebug's
> instrumented code replacing Edebug's functions edebug-enter,
> edebug-before and edebug-after with Testcover's functions (some with
> different call signatures), and at the same time determining which
> forms might only return a single value and treating them
> differently. Since it uses car and cdr to parse and rewrite the code,
> it is consequently difficult to read and modify.

Thanks, I have a few comments below.  Please let others to comment for
a week or so before installing on master.

> 
> My original goal was to change testcover-reinstrument to use pcase,
> but along the way I realized that I could simplify it and not only fix
> all the bugs where one of Edebug's functions gets left in the
> instrumented code, but remove the possibility of undiscovered or
> future ones by not doing code rewriting and instead adding a hook
> mechanism to make Edebug's functions call Testcover's. So in these
> patches testcover-reinstrument has become testcover-analyze-coverage,
> which uses pcase to walk Edebug's instrumented code and save the
> results of its analysis of single-value forms in Edebug's code
> coverage vector.
> 
> 
> [2:text/plain Hide 
> Save:0001-Allow-Edebug-s-instrumentation-to-be-used-for-other-.patch (10kB)]
> 
> >From c0e8293a9b4919b22e3d409acddc9bde49021bc8 Mon Sep 17 00:00:00 2001
> From: Gemini Lasswell <gazally@runbox.com>
> Date: Sat, 16 Sep 2017 14:23:35 -0700
> Subject: [PATCH 1/2] Allow Edebug's instrumentation to be used for other
>  purposes
> 
> * lisp/emacs-lisp/edebug.el:
> (edebug-after-instrumentation-functions)
> (edebug-new-definition-functions): New hook variables.
> (edebug-behavior-alist): New variable.
> (edebug-read-and-maybe-wrap-form): Run a hook after a form is
> wrapped.
> (edebug-make-form-wrapper): Run a hook after a definition is
> wrapped.
> (edebug-default-enter): New name for edebug-enter.
> (edebug-enter): New function which changes behavior of Edebug based
> on symbol property 'edebug-behavior and edebug-behavior-alist.
> (edebug-run-slow, edebug-run-fast): Modify edebug-behavior-alist.
> ---
>  lisp/emacs-lisp/edebug.el | 154 
> ++++++++++++++++++++++++++++++----------------
>  1 file changed, 100 insertions(+), 54 deletions(-)
> 
> diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
> index dbc56e272f..ca1f55cb6a 100644
> --- a/lisp/emacs-lisp/edebug.el
> +++ b/lisp/emacs-lisp/edebug.el
> @@ -1065,6 +1065,29 @@ 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
> +;; 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
> +called with one argument, the symbol associated with the
> +definition, which may be the actual symbol defined or one
> +generated by Edebug.")

These hooks should be documented in the ELisp manual and in NEWS.

Also, please have an empty line between the defvar's.

> @@ -1330,10 +1354,7 @@ edebug-make-form-wrapper
>        form-data-entry edebug-def-name ;; in case name is changed
>        form-begin form-end))
>  
> -      ;;    (message "defining: %s" edebug-def-name) (sit-for 2)
>        (edebug-make-top-form-data-entry form-data-entry)
> -      (message "Edebug: %s" edebug-def-name)
> -      ;;(debug edebug-def-name)

Why are you removing this?  It looks like debugging code that could be
useful to someone, no?

> +(defun edebug-announce-definition (def-name)
> +  "Announce Edebug's processing of DEF-NAME."
> +  (message "Edebug: %s" def-name))

This new function is not mentioned in the commit log message.

> -(defun edebug-enter (function args body)
> +(defun edebug-enter (func args body)

This function is indicated as "new" in the commit log, but it isn't
new.

> +(defalias 'edebug-before nil
> +  "Function called by Edebug before a form is evaluated.
> +See `edebug-behavior-alist' for implementations.")
> +(defalias 'edebug-after nil
> +  "Function called by Edebug after a form is evaluated.
> +See `edebug-behavior-alist' for implementations.")

These are not in the commit log.

> +** Testcover
> +
> +---
> +*** If you've tried to use Testcover before and were blocked by
> +unhelpful error messages, give it another try.  Many bugs have been
> +fixed.

Please make this a more informative entry.  I would remove the 1st
sentence, and instead describe what new features are available and
what could users do with them.  Since this is not documented in the
manuals (and you suggest to leave it at that with the "---" marker),
the NEWS entry should do a better job describing the changes.  (You
don't need to describe which bugs were fixed, only the new and changed
behavior.)

> +;;; Coverage Analysis
> +
> +;; The top level function for initializing code coverage is
> +;; `testcover-analyze-coverage', which recursively walks the form it is
> +;; passed, which should have already been instrumented by
> +;; edebug-read-and-maybe-wrap-form, and initializes the associated
> +;; code coverage vectors, which should have already been created by
> +;; `edebug-clear-coverage'.
> +;;
> +;; The purpose of the analysis is to identify forms which can only
> +;; ever return a single value. These forms can be considered to have
> +;; adequate code coverage even if only executed once. In addition,
> +;; forms which will never return, such as error signals, can be
> +;; identified and treated correctly.

This text uses one space between sentences, whereas our convention is
to use 2.

Thanks again for working on this.