Subject: bug#50599: [PATCH] Don't recommend against "\[...]" substitutions for performance

From: Stefan Kangas <[email-masked]>
Date: Wed, 15 Sep 2021 08:27:55 +0200

Severity: minor

In `(elisp) Documentation Tips', we read:

     It is not practical to use ‘\\[...]’ very many times, because
     display of the documentation string will become slow.  So use this
     to describe the most important commands in your major mode, and
     then use ‘\\{...}’ to display the rest of the mode’s keymap.

When testing this on my machine on a docstring with a large number of
substitutions (107), I get the following (in "emacs -Q"):

(progn (require 'ibuffer)
       (let ((times 100))
         (/ (car (benchmark-run
                     times (documentation 'ibuffer-mode)))
            times)))

    => 0.00499586008

When I increase the number of substitutions in that docstring to around 1000 (by
duplicating the docstring 10 times), I get:

    => 0.05029239337

This is 10 times slower, but still fast enough that it does not matter much.
It also suggests that this is O(N) in time.

My conclusion is that the above recommendation in `(elisp) Documentation Tips'
is irrelevant these days, and I suggest to remove it.

Please see the attached patch.
From d3bca2d60f07876a562378f1b56844770ab87404 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
Date: Wed, 15 Sep 2021 08:18:20 +0200
Subject: [PATCH] Don't recommend against using "\[...]" substitutions many
 times

* doc/lispref/tips.texi (Documentation Tips): Don't recommend against
using many "\[...]" substitutions for reasons of performance.  This
recommendation is no longer relevant, as this is more than fast enough
on modern machines.

* lisp/emacs-lisp/checkdoc.el (checkdoc-max-keyref-before-warn):
Add new valid value nil meaning to never warn about too many command
substitutions.  Make nil the new default.
(checkdoc-this-string-valid-engine): Respect above new value.
---
 doc/lispref/tips.texi       |  5 -----
 lisp/emacs-lisp/checkdoc.el | 36 +++++++++++++++++++++---------------
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/doc/lispref/tips.texi b/doc/lispref/tips.texi
index f0eb1079ca..7c3c9b1c2a 100644
--- a/doc/lispref/tips.texi
+++ b/doc/lispref/tips.texi
@@ -789,11 +789,6 @@ Documentation Tips
 @samp{\\<@dots{}>} should be the name of the variable containing the
 local keymap for the major mode.
 
-It is not practical to use @samp{\\[@dots{}]} very many times, because
-display of the documentation string will become slow.  So use this to
-describe the most important commands in your major mode, and then use
-@samp{\\@{@dots{}@}} to display the rest of the mode's keymap.
-
 @item
 For consistency, phrase the verb in the first sentence of a function's
 documentation string as an imperative---for instance, use ``Return the
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index e10ea736cd..e0797f2b92 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -249,11 +249,17 @@ checkdoc-ispell-lisp-words
   "List of words that are correct when spell-checking Lisp documentation.")
 ;;;###autoload(put 'checkdoc-ispell-list-words 'safe-local-variable #'checkdoc-list-of-strings-p)
 
-(defcustom checkdoc-max-keyref-before-warn 10
-  "The number of \\ [command-to-keystroke] tokens allowed in a doc string.
+(defcustom checkdoc-max-keyref-before-warn nil
+  "If non-nil, number of \\\\=[command-to-keystroke] tokens allowed in a doc string.
 Any more than this and a warning is generated suggesting that the construct
-\\ {keymap} be used instead."
-  :type 'integer)
+\\\\={keymap} be used instead.  If the value is nil, never warn.
+
+It used to not be practical to use `\\\\=[...]' very many times,
+because display of the documentation string would become slow.
+This is typically not an issue on modern machines."
+  :type '(choice (const nil)
+                 integer)
+  :version "28.1")
 
 (defcustom checkdoc-arguments-in-order-flag nil
   "Non-nil means warn if arguments appear out of order.
@@ -1543,17 +1549,17 @@ checkdoc-this-string-valid-engine
 	       " embedded in doc string.  Use \\\\<keymap> & \\\\[function] "
 	       "instead")
 	      (match-beginning 1) (match-end 1) t))))
-     ;; It is not practical to use `\\[...]' very many times, because
-     ;; display of the documentation string will become slow.  So use this
-     ;; to describe the most important commands in your major mode, and
-     ;; then use `\\{...}' to display the rest of the mode's keymap.
-     (save-excursion
-       (if (and (re-search-forward "\\\\\\\\\\[\\w+" e t
-				   (1+ checkdoc-max-keyref-before-warn))
-		(not (re-search-forward "\\\\\\\\{\\w+}" e t)))
-	   (checkdoc-create-error
-	    "Too many occurrences of \\[function].  Use \\{keymap} instead"
-	    s (marker-position e))))
+     ;; It used to not be practical to use `\\[...]' very many times,
+     ;; because display of the documentation string would become slow.
+     ;; This is typically not an issue on modern machines.
+     (when checkdoc-max-keyref-before-warn
+       (save-excursion
+         (if (and (re-search-forward "\\\\\\\\\\[\\w+" e t
+                                     (1+ checkdoc-max-keyref-before-warn))
+                  (not (re-search-forward "\\\\\\\\{\\w+}" e t)))
+             (checkdoc-create-error
+              "Too many occurrences of \\[function].  Use \\{keymap} instead"
+              s (marker-position e)))))
      ;; Ambiguous quoted symbol.  When a symbol is both bound and fbound,
      ;; and is referred to in documentation, it should be prefixed with
      ;; something to disambiguate it.  This check must be before the
-- 
2.30.2