Re: Merging scratch/substitute-command-keys to master

[Stefan Monnier <monnier@iro.umontreal.ca>]

> One obvious concern is performance.  Here is what I've used to
> benchmark it:
>
> (with-temp-buffer
>     (dired-mode)
>     (let* ((times 100)
>            (result (benchmark-run times
>                      (documentation 'dired-mode))))
>       (cl-mapcar '/ result (make-list 3 (float times)))))
>
> Old C version:     (0.02171680510 0.2  0.01730526807)
> New Lisp version:  (0.02775125134 0.24 0.01882684842)

LGTM!

> [It would be useful if someone could help verify the above results.]

I don't have a clean baseline against which to run such tests, but I can
confirm that the speed is comparable (I do see a 50% increase of
calls to the GCs (instead of the 20% increase you're seeing), but
I don't think it's something we should worry about).

> Finally, note that I didn't yet rip out the old C code, and that the
> branch is currently based on the master branch from May.

AFAICT it applies cleanly to the current master.

See comments below.  Other than those details, LGTM,

> (BTW, do we generally merge branches as-is with their full history or
> do we squash them and push as a single commit?)

We generally do either of those or yet something else (e.g. rebase,
with or without cleaning up the history).
I like to just merge, personally.


        Stefan


I used `git diff HEAD...origin/scratch/substitute-command-keys`:

> diff --git a/lisp/help.el b/lisp/help.el
> index b7d867eb70..196c431607 100644
> --- a/lisp/help.el
> +++ b/lisp/help.el
> @@ -147,12 +147,12 @@ help-print-return-message
>  		      pop-up-frames
>  		      (special-display-p (buffer-name standard-output)))
>  		     (setq help-return-method (cons (selected-window) t))
> -		     ;; If the help output buffer is a special display buffer,
> -		     ;; don't say anything about how to get rid of it.
> -		     ;; First of all, the user will do that with the window
> -		     ;; manager, not with Emacs.
> -		     ;; Secondly, the buffer has not been displayed yet,
> -		     ;; so we don't know whether its frame will be selected.
> +                     ;; If the help output buffer is a special display buffer,
> +                     ;; don't say anything about how to get rid of it.  First of
> +                     ;; all, the user will do that with the window manager, not
> +                     ;; with Emacs.  Secondly, the buffer has not been displayed
> +                     ;; yet, so we don't know whether its frame will be
> +                     ;; selected.
>  		     nil)
>  		    ((not (one-window-p t))
>  		     (setq help-return-method

This looks like a spurious change.

> +Each substring of the form \[COMMAND] is replaced by either a

In Emacs's `master` the ELisp major mode should show you those \[ with
a bright red color on the backslash because it does nothing.

> +Each substring of the form \{MAPVAR} is replaced by a summary of

Same here (and a few other places).

> +(defvar internal--seen)
[...]
> +(defun describe-map-tree (startmap partial shadow prefix title no-menu
> +                                   transl always-title mention-shadow)
[...]
> +    (setq internal--seen nil)
[...]

This `setq` will give a global value to the var.
So you might as well also give it a global value in the `defvar`.
Also, I'd prefer it if the variable used a prefix that's related to the
file where it's defined (i.e. `help--`), although it's true that
`help.el` is one of those files that doesn't even try to pretend it
obeys the prefix rule.

> +(defvar help--previous-description-column)

Same comment here: give it a default value.

> +;;; The Lisp version is 100 times slower than the C equivalent:

I think you meant "This" rather then "The".

> +;; (defun describe-keymap-or-char-table
[...]
> +;;       (let* ((val (aref keymap idx))

`aref` will signal an error when applied to a keymap.
So your `keymap` variable doesn't hold a keymap, and so your function
name is misleading.

Also this function should probably have a "help--" prefix because it
should stay as an internal detail.

> +  enum text_quoting_style style = text_quoting_style();
> +  switch (style)

You need a space before "()", and I think you can dispense with the
`style` variable and directly do `switch (text_quoting_style ())`.

> +  return get_keyelt (object, NILP(autoload) ? false : true);
                                   ^^
                                   SPC
 
> +  Lisp_Object msg;

I think you don't need/want this var here.

> +    {
> +      msg = build_unibyte_string("Key translations");
        ^                         ^^
    Lisp_Object                   SPC

Your compiler will be just as happy with many such "small scope" vars as
with your larger scope var.

Also, you might want to use `AUTO_STRING` here.

> @@ -2793,8 +2815,11 @@ DEFUN ("describe-buffer-bindings", Fdescribe_buffer_bindings, Sdescribe_buffer_b

BTW, I see that `describe-buffer-bindings` is a natural next candidate
for ELispification ;-)

> +DEFUN ("describe-keymap-or-char-table", Fdescribe_keymap_or_char_table, 
[...]
> +  CHECK_VECTOR_OR_CHAR_TABLE (vector);

The code says VECTOR_OR_CHAR_TABLE, so your function's name might want
to do the same.  Or maybe call it `describe-vector` since it seems to be
a thin wrapper around that function.

> @@ -3654,6 +3742,10 @@ syms_of_keymap (void)
>  be preferred.  */);
>    Vwhere_is_preferred_modifier = Qnil;
>    where_is_preferred_modifier = 0;
> +  DEFVAR_LISP ("internal--seen", Vinternal_seen,
> +	       doc: /* List of seen keymaps.
> +This is used for internal purposes only.  */);
> +  Vinternal_seen = Qnil;

This doesn't belong here, since this var is not used in the C code AFAICT.