Merging scratch/substitute-command-keys to master

Merging scratch/substitute-command-keys to master

[Stefan Kangas]

I've been working on translating `substitute-command-keys' from C to
Lisp, and I think it's getting ready to merge to master.  Please see the
branch scratch/substitute-command-keys.

Note that this is a rather close 1 to 1 translation from C to Lisp, so
there should also exist many opportunities to take better advantage of
a more expressive language.

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)

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

I hope the above figures are acceptable.  Hopefully, we will be able
to improve the implementation to be more performant over time.

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.

Thanks in advance for any comments and reviews.

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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 17 Oct 2020 14:03:25 +0000
> 
> (BTW, do we generally merge branches as-is with their full history or do
> we squash them and push as a single commit?)

It's up to you.

Thanks for working on this.

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

[Stefan Monnier]

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

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

[Drew Adams]

> I've been working on translating `substitute-command-keys' from C to
> Lisp, and I think it's getting ready to merge to master.  

Sounds good. Thanks for working on this.

I hope you'll consider adding help buttons for key descriptions,
as is done in my function `help-substitute-command-keys' (in
`help-fns+.el), which I think you've seen.  Doc:

 (help-substitute-command-keys STRING &optional ADD-HELP-BUTTONS)

 Same as `substitute-command-keys', but optionally adds buttons for help.
 Non-nil optional arg ADD-HELP-BUTTONS does that, adding buttons to key
 descriptions, which link to the key's command help.

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

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

>> I've been working on translating `substitute-command-keys' from C to
>> Lisp, and I think it's getting ready to merge to master.
>
> Sounds good. Thanks for working on this.
>
> I hope you'll consider adding help buttons for key descriptions,
> as is done in my function `help-substitute-command-keys' (in
> `help-fns+.el), which I think you've seen.  Doc:
>
>  (help-substitute-command-keys STRING &optional ADD-HELP-BUTTONS)
>
>  Same as `substitute-command-keys', but optionally adds buttons for help.
>  Non-nil optional arg ADD-HELP-BUTTONS does that, adding buttons to key
>  descriptions, which link to the key's command help.

Thanks for commenting.

I specifically avoided making any functional changes on this branch; my
main concern was correctness.  But it should indeed be much easier to
add features like the above once this is merged.

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

[Stefan Kangas]

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

> I don't have a clean baseline against which to run such tests, but I can
> confirm that the speed is comparable

Thanks, that information is very useful.

> See comments below.  Other than those details, LGTM,
[...]
> 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.

Thank you kindly for the review.

I've fixed all your comments unless indicated otherwise below.  I'm
currently working on cleaning up the history of the branch and will push
a new version of the branch, that I then plan to merge to the master
branch.

>> +;; (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.

Right.  I probably picked up this terminological confusion from
describe_vector, where the documentation says, a bit confusingly, that
"KEYMAP_P is 1 if vector is known to be a keymap".  But of course a
vector is never a keymap, at least not according to `keymapp'.  So the
argument here should better be called `vector' (as is the case in
the describe_vector function on which this is based).

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

See the discussion on this name below.

>> +    {
>> +      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.

(I fixed the first part here.)

I tried using AUTO_STRING at first, but these strings are passed to
Qdescribe_map_tree so that leads to segfault if there is a GC.
While investigating that, I learned that AUTO_STRINGs are allocated on the
stack (I guess without the proper reference counters?) and shouldn't be
passed to Lisp code.

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

Indeed!

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

The problem is that there is already a `describe-vector' with a
different calling convention.  So perhaps we should just go with
`describe-vector-or-char-table' (clunky as it may be) if no one has a
better proposal.  Or maybe `help--describe-vector' is better?  Or the
iniquitous `help--describe-vector-or-char-table'?

Thanks again for reviewing.

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

[Stefan Monnier]

> I tried using AUTO_STRING at first, but these strings are passed to
> Qdescribe_map_tree so that leads to segfault if there is a GC.

Oh, right!  Oops!

>> 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.
>
> The problem is that there is already a `describe-vector' with a
> different calling convention.  So perhaps we should just go with
> `describe-vector-or-char-table' (clunky as it may be) if no one has a
> better proposal.  Or maybe `help--describe-vector' is better?  Or the
> iniquitous `help--describe-vector-or-char-table'?

I think this function's name deserves a double dash, in any case.


        Stefan

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

[Stefan Kangas]

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

>> The problem is that there is already a `describe-vector' with a
>> different calling convention.  So perhaps we should just go with
>> `describe-vector-or-char-table' (clunky as it may be) if no one has a
>> better proposal.  Or maybe `help--describe-vector' is better?  Or the
>> iniquitous `help--describe-vector-or-char-table'?
>
> I think this function's name deserves a double dash, in any case.

OK, I'll go with the shorter `help--describe-vector'.

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

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

>> See comments below.  Other than those details, LGTM,
>
> I've fixed all your comments unless indicated otherwise below.  I'm
> currently working on cleaning up the history of the branch and will push
> a new version of the branch, that I then plan to merge to the master
> branch.

I've now pushed a (hopefully) final version of the branch to
scratch/substitute-command-keys, fixing Stefan M's comments, rebased on
top of current master and with the old C code finally removed.