Re: Performance degradation from long lines

[Phil Sainty <psainty@orcon.net.nz>]

On 13/01/19 12:08 AM, Eli Zaretskii wrote:
>> http://git.savannah.gnu.org/cgit/emacs.git/log?h=scratch/so-long
> 
> Thanks.  I think we can land this on master, but see a few minor
> comments below, mostly about documentation.
> 
> Please use quoting 'like this' consistently [in NEWS].

Fixed.


>   +;; This library advises `set-auto-mode' (in order to react after Emacs has
>   +;; chosen the major mode for a buffer), and `hack-local-variables' (so that we
>   +;; may behave differently when a file-local mode is set).  In earlier versions
>   +;; of Emacs (< 26.1) we also advise `hack-one-local-variable' (to prevent a
>   +;; file-local mode from restoring the original major mode if we had changed it).
> 
> I'd prefer not to use advices in bundled code.  Is it feasible to
> instead include hooks in Emacs that so-long-mode could use?  If the
> problem is older versions of Emacs, I'm okay with using advices only
> for those old versions.

This harks back to my earlier comments here:
https://lists.gnu.org/archive/html/emacs-devel/2018-10/msg00551.html

I was subsequently convinced by the argument that building the GNU ELPA
release directly from the core version for Emacs 27 was the sensible
approach, so the library as-is is intended to be backwards-compatible
(minimum version is presently 24.4).

I'm sure that we could introduce some new changes and hooks to 27 to
avoid the need for the advice, but I think that could be version 1.1.


>   +(defgroup so-long nil
>   +  "Prevent unacceptable performance degradation with very long lines."
>   +  :prefix "so-long"
>   +  :group 'convenience)
>   +
>   +(defcustom so-long-threshold 250
>   +  "Maximum line length permitted before invoking `so-long-function'.
>   +
>   +See `so-long-detected-long-line-p' for details."
>   +  :type 'integer
>   +  :package-version '(so-long . "1.0")
>   +  :group 'so-long)
> 
> Please add :version tags to all the customizable options.

It hadn't occurred to me that the defgroup should specify a version,
but I can add that.

I thought that :package-version functioned as an alternative to
:version which worked both for the core library and for the GNU ELPA
package for earlier emacs versions?

Libraries like rst.el and mh-e.el seem to use it this way.

I already have:

(add-to-list 'customize-package-emacs-version-alist
             '(so-long ("1.0" . "27.1")))

And :package-version '(so-long . "1.0") for all options.

Do I need to add :version "27.1" as well?


>   +(defcustom so-long-target-modes
>   +  '(prog-mode css-mode sgml-mode nxml-mode)
>   +  "`so-long' affects only these modes and their derivatives.
> 
> I've heard complaints from users of JASON and JavaScript -- should the
> default value cover those as well?

JSON rather than JASON, I imagine?

js-mode is derived from prog-mode, so both of those are covered.


>   +(defun so-long-action-alist-setter (option value)
>   +  "The :set function for `so-long-action-alist'."
> 
> For non-internal functions, please reference the arguments in the
> first line of the doc string.

That's pretty internal.  I'll rename that and `so-long-action-type'
with a so-long-- prefix.


>   +(defun so-long-change-major-mode ()
>   +  "Ensures that `so-long-mode' knows the original `major-mode'
>   +even when invoked interactively.
> 
> "Ensure", to be consistent with our style of writing the first
> sentence of the doc strings.

Done.

> Also, please make the first sentence fit on a single line -- this
> is important for Apropos commands, which only display one line.

In this instance I had intentionally wrapped the line early so that
the first line was coherent on its own, even though it was part of a
longer sentence.  i.e.:

"Ensures that `so-long-mode' knows the original `major-mode'"

If that's not sufficient, I can rephrase it over multiple sentences.


>   +(defun so-long-menu ()
>   +  "Dynamically generate the \"So Long\" menu."
>   +  ;; (info "(elisp) Menu Example")
> 
> How about providing some help-echo for this menu?

I'd tried that initially, but I found it so glitchy that I removed it.
As I recall, there was a tendency for a blank popup to appear a lot of
the time, which wasn't useful, and wasn't something I wanted to debug.

The Help sub-menu contains an option for reading the Commentary,
which covers the various menu items, so I'm happy with that for
documentation.


>   +(defun so-long-menu-item-revert ()
>   +  "Invoke `so-long-revert'."
> 
> This doc string is not very informative, and neither is that of
> so-long-revert.  How about adding more detailed description here?
> I believe this is an important command that users will want to know
> about.

True.  I don't think there's very much to add, but I shall elaborate
somewhat on the latter's documentation.

I think the `so-long-menu-item-revert' docstring can remain as a
simple cross-reference to `so-long-revert', or else I could do this:

;; Duplicate the `so-long-revert' documentation for the menu item.
(put 'so-long-menu-item-revert 'function-documentation
     (documentation 'so-long-revert t))


-Phil