bug#44858: [PATCH] Make byte-compiler warn about wide docstrings

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 10 Dec 2020 14:59:16 -0600
> Cc: 44858@debbugs.gnu.org
> 
> +** byte compiler

Please capitalize "Byte".

> +*** The byte-compiler now warns about too wide documentation strings.
> +By default, it will warn if a documentation string is wider than the
> +largest of 80 characters or 'fill-column'.  See the new user
> +option 'byte-compile-docstring-max-column'.

The last sentence will be more useful if you make it more informative,
e.g.

  This is controlled by the new user option
  'byte-compile-docstring-max-column'.

> +(defvar byte-compile--wide-docstring-substitution-len 3
> +  "Substitution width used in `byte-compile--wide-docstring-p'.")

The doc string should be more detailed.  Let's at least explain what
hides behind "substitution", and say that the value is a heuristic.

> +(defcustom byte-compile-docstring-max-column 80
> +  "Length that a doc string can be before the byte-compiler reports a 
> warning."

I suggest "width", not "length", since you both use the former
elsewhere, and because "column" goes better with "width".

Also, "can be before" is not the clearest way of saying this.  I
suggest

    Recommended maximum width of doc string lines.
  The byte-compiler will emit a warning for doc-string lines
  wider than this.  If `fill-column' has a larger value, it will
  override this variable.

> +(defun byte-compile-docstring-length-warn (form)
> +  "Warn if documentation string of FORM is too wide.
> +It is too wide if it is longer than the largest of `fill-column'
> +and `byte-compile-docstring-max-column'."

"It is too wide if it is longer" is inaccurate, I think, because we
actually test each _line_ of the doc string against the limit, while
the above seems to imply the overall length of the doc string (which
could be multi-line) is tested.  So:

  It is too wide if some of its lines is wider than the largest of...

> --- /dev/null
> +++ b/test/lisp/emacs-lisp/bytecomp-resources/warn-long-docstring-autoload.el
> @@ -0,0 +1,3 @@
> +;;; -*- lexical-binding: t -*-
> +(autoload 'foox "foo"
> +  
> "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")

I see that none of the tests use multi-line doc strings, is that
right?  If so, I think it would be good to have at least a couple, one
where the first line is too wide, and another where some line other
than the first is too wide.  Also, what about testing the fact that
fill-column overrides the value of the new option?

Thanks.