[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Performance degradation from long lines

From: Eli Zaretskii
Subject: Re: Performance degradation from long lines
Date: Sat, 12 Jan 2019 13:08:21 +0200

> From: Phil Sainty <address@hidden>
> Cc: Eli Zaretskii <address@hidden>, Stefan Monnier <address@hidden>,
>  mithraeum <address@hidden>
> Date: Sat, 12 Jan 2019 14:03:14 +1300
> I've pushed a branch scratch/so-long with release-candidate code for
> including so-long.el in Emacs 27 (and for the GNU ELPA package).
> 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.

  +** so-long.el helps to mitigate performance problems with long lines.
  +When `so-long-enable' has been called, visiting a file with very long
  +lines will (subject to configuration) cause the user's preferred
  +`so-long-action' to be automatically invoked (by default, the buffer's
  +major mode is replaced by `so-long-mode').  In extreme cases this can
  +prevent delays of several minutes, and make Emacs responsive almost
  +immediately.  Type 'M-x so-long-commentary' for full documentation.

Please use quoting 'like this' consistently.

  +;; 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 
  +;; may behave differently when a file-local mode is set).  In earlier 
  +;; 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 

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.

  +(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.

  +(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?

  +(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.

  +(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.  Also, please make the first sentence fit
on a single line -- this is important for Apropos commands, which only
display one line.

  +(defun so-long-menu ()
  +  "Dynamically generate the \"So Long\" menu."
  +  ;; (info "(elisp) Menu Example")

How about providing some help-echo for this menu?

  +(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

Thanks again for working on this.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]