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

[Stefan Kangas <stefan@marxist.se>]

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.