[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Performance degradation from long lines
Re: Performance degradation from long lines
Sun, 10 Mar 2019 23:22:02 +1300
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1
On 13/01/19 12:08 AM, Eli Zaretskii wrote:
> 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].
> +;; 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
> +;; 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:
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:
'(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.
> 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.
> 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
> +(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
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))