On 6/23/2021 6:58 AM, Stefan Monnier wrote:
>>> +         (sort (cdr keymap)
>>> +               (lambda (a b)
>>> +                 (string< (bindings--menu-item-string (cdr-safe a))
>>> +                          (bindings--menu-item-string (cdr-safe b)))))))
>>> +    (setcdr keymap menu-items)
>>
>> I'm slightly worried about this, though -- will altering the keymap
>> structure have any adverse side effects?  I don't think so, and testing
>> a bit doesn't seem to reveal anything obvious, but we should be on the
>> lookup.
> 
> Altering the keymap by side-effect is not dangerous in and of itself
> (`define-key` does it as well).
> But the above code will mess things up when applied to a keymap that has
> a parent keymap.
> 
> It will also fail to do its job on menu keymaps which use a vector
> rather or on composed keymaps (which can be
> created "implicitly", e.g. when looking up `menu-bar` in a keymap which
> has a `menu-bar` binding and which additionally inherits from a keymap
> that also has a `menu-bar` binding).
> 
> To solve those issues I think one would have to `map-keymap` and return
> a brand new keymap rather than modify it in-situ, AFAICT.

Attached is a patch that uses `map-keymap'. I've modeled the 
implementation on `keymap-canonicalize', so hopefully it's 
sufficiently-correct. The patch doesn't do anything special about 
char-ranges, but I'm not sure that matters for menus.

One difference in this patch is that `mode-line-mode-menu' itself is no 
longer updated after sorting. It just returns a new keymap. The 
performance is the same as before, which is good, and this is probably a 
bit safer too.

- Jim