* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library [not found] ` <20190107065208.BA36C21736@vcs0.savannah.gnu.org> @ 2019-01-10 18:07 ` Stefan Monnier 2019-01-12 2:20 ` Phil Sainty 0 siblings, 1 reply; 33+ messages in thread From: Stefan Monnier @ 2019-01-10 18:07 UTC (permalink / raw) To: emacs-devel; +Cc: Phil Sainty > +FIXME: +++ once all documentation has been added. No need for this FIXME: the whole point of this system is that you don't need to do anything special to mark entries as "not yet handled" (any `**` header without a preceding `---` or `+++` has not been handled yet), so it's "fail safe". > +;; Installation > +;; ------------ > +;; And add the following to your init file to enable the library: > +;; > +;; (when (require 'so-long nil :noerror) > +;; (so-long-enable)) I think the above should just be (so-long-auto-mode 1) and so-long-auto-mode (just my suggestion for a name, feel free to chose something else) should be an autoloaded global minor mode which activates so-long-mode. On the design side, I think you should merge `so-long` and `so-long-mode` into a single function and make that function a (buffer-local) minor-mode (i.e. not have any major mode, just use fundamental-mode instead). Making it a minor mode rather than a major-mode will also make it easier to remember the previous major-mode without any need for a change-major-mode-hook hack. > +(defvar so-long-enabled t > + "Set to nil to prevent `so-long' from being triggered automatically.") Loading a file should not change the behavior of Emacs, so the default should be nil unless/until we decide to preload this package. Rather than defadvice, please use advice-add. Other than that, looks great, thanks. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-01-10 18:07 ` [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library Stefan Monnier @ 2019-01-12 2:20 ` Phil Sainty 2019-01-12 15:11 ` Stefan Monnier 0 siblings, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-01-12 2:20 UTC (permalink / raw) To: Stefan Monnier, emacs-devel Hi Stefan, Thanks for the feedback. I didn't spot this until after I'd posted my last message to the "Performance degradation from long lines" thread. On 11/01/19 7:07 AM, Stefan Monnier wrote: >> +FIXME: +++ once all documentation has been added. > > No need for this FIXME: the whole point of this system is that you don't > need to do anything special to mark entries as "not yet handled" (any `**` > header without a preceding `---` or `+++` has not been handled yet), so > it's "fail safe". That FIXME was just for me -- I knew that I wanted to add something to the manual, but hadn't done so yet, and didn't want to mark the NEWS item +++ until I'd added that, and I didn't want to forget to mark it +++ once I'd done so :) The current branch has this fixed. >> +;; Installation >> +;; ------------ >> +;; And add the following to your init file to enable the library: >> +;; >> +;; (when (require 'so-long nil :noerror) >> +;; (so-long-enable)) > > I think the above should just be > > (so-long-auto-mode 1) > > and so-long-auto-mode (just my suggestion for a name, feel free to > chose something else) should be an autoloaded global minor mode which > activates so-long-mode. I actually spent some time experimenting with that idea last month, as it seemed like a good idea to me as well, but it got sufficiently awkward that I dropped the notion as being more hassle than it was worth. If I define a global mode, then there's another variable which is ostensibly tracking the 'enabled' state of the functionality, which seemed ugly. I tried using :variable in the mode definition to make that use `so-long-enabled' internally, but then in testing I found that I could de-sync the mode and that variable (I forget how, but possibly by calling `so-long-disable' which I need to keep for backwards-compatibility), and then it just struck me that I was making more work and a bit of a headache for myself in an effort to allow people to add '(global-so-long-mode)' in their init files instead of '(so-long-enable)' and I immediately lost all interest in spending more time on achieving that. `so-long-enable' is autoloaded, so I could drop the test for (require 'so-long nil :noerror) in the Commentary. That's a hold- over from the earlier releases, and it seemed harmless to keep, but I'm happy to remove it, as it's not needed for either 27 or ELPA. > On the design side, I think you should merge `so-long` and > `so-long-mode` into a single function and make that function > a (buffer-local) minor-mode (i.e. not have any major mode, just use > fundamental-mode instead). Making it a minor mode rather than > a major-mode will also make it easier to remember the previous > major-mode without any need for a change-major-mode-hook hack. These changes are too significant, so I don't wish to do any of that. There's backwards-compatibility with the earlier releases to consider (so-long-mode has always been a major mode); and I also see a benefit to retaining a major mode option (I have a use-case for using it as a file-local 'mode' on one project, and I've had cause to suggest the same thing to other users). I did consider turning `so-long' into a `so-long-minor-mode' at one point, but I think it either seemed like a fairly needless change, or possibly the idea of implementing a feature which was designed to mess with minor mode states *as* a minor mode seemed wrong to me. 'M-x so-long' is also shorter :) >> +(defvar so-long-enabled t >> + "Set to nil to prevent `so-long' from being triggered automatically.") > > Loading a file should not change the behavior of Emacs, so the default > should be nil unless/until we decide to preload this package. That var does nothing at all if (so-long-enable) hasn't been called; but offhand I think it's fine to set it nil by default, so I'll do that. > Rather than defadvice, please use advice-add. I know defadvice and I don't know advice-add, so perhaps someone else can write those changes. Such changes will need to work in Emacs 25 at least, and ideally back to 24.3 (but as I can no longer build 24.x on my machine, I've not actually tested the current library on that version myself, so I'm not certain that I've retained compatibility with 24.3 in this release). I have vague recollections that advice-add is very different when it comes to ad-get-arg, so note in particular that the signature to hack-local-variables changed in 26.1. That's not an issue for the advice I've written, but if it would be for nadvice, then that needs to be accounted for in any rewrite. > Other than that, looks great, thanks. Cheers, -Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-01-12 2:20 ` Phil Sainty @ 2019-01-12 15:11 ` Stefan Monnier 2019-01-12 21:01 ` Stefan Monnier 2019-04-14 13:09 ` Phil Sainty 0 siblings, 2 replies; 33+ messages in thread From: Stefan Monnier @ 2019-01-12 15:11 UTC (permalink / raw) To: Phil Sainty; +Cc: emacs-devel > If I define a global mode, then there's another variable which is > ostensibly tracking the 'enabled' state of the functionality, which > seemed ugly. I don't see why. If you don't want to get rid of the so-long-enabled variable, then just make it a defvaralias and you're done. > that I could de-sync the mode and that variable (I forget how, but > possibly by calling `so-long-disable' which I need to keep for > backwards-compatibility), Redefine so-long-enable as an obsolete function that just calls (so-long-auto-mode 1). Side note: I mistakenly thought so-long was pretty much a brand new package. How far back does it go? > and then it just struck me that I was making more work and a bit of > a headache for myself in an effort to allow people to add > '(global-so-long-mode)' in their init files instead of > '(so-long-enable)' and I immediately lost all interest in spending > more time on achieving that. The issue is consistency (a minor mode is a standard interface and UI to enable&disable&test a feature) and ease of use (a global minor mode can be enabled via Customize). > `so-long-enable' is autoloaded, so I could drop the test for > (require 'so-long nil :noerror) in the Commentary. That's a hold- > over from the earlier releases, and it seemed harmless to keep, Users will copy it and wonder why Emacs has to be so complicated, and it will then fail the day the file is renamed. > but I'm happy to remove it, as it's not needed for either 27 or ELPA. That would be good. >> On the design side, I think you should merge `so-long` and >> `so-long-mode` into a single function and make that function >> a (buffer-local) minor-mode (i.e. not have any major mode, just use >> fundamental-mode instead). Making it a minor mode rather than >> a major-mode will also make it easier to remember the previous >> major-mode without any need for a change-major-mode-hook hack. > > These changes are too significant, so I don't wish to do any of that. I think they're just some minor shuffling of code. > There's backwards-compatibility with the earlier releases to consider > (so-long-mode has always been a major mode); Then use another name for the minor mode and make so-long-mode be the combination of fundamental-mode + so-long-minor-mode. > and I also see a benefit to retaining a major mode option (I have > a use-case for using it as a file-local 'mode' on one project, and > I've had cause to suggest the same thing to other users). Minor modes can also be used on the "mode:" thingy, tho I don't think that should make a big difference. > I did consider turning `so-long' into a > `so-long-minor-mode' at one point, but I think it either seemed like > a fairly needless change, or possibly the idea of implementing a > feature which was designed to mess with minor mode states *as* a minor > mode seemed wrong to me. 'M-x so-long' is also shorter :) The point of a minor mode is to provide a standard UI to enable *and* disable something. >> Rather than defadvice, please use advice-add. > I know defadvice and I don't know advice-add, so perhaps someone else > can write those changes. E.g. (defadvice hack-local-variables (after so-long--file-local-mode disable) "..." ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, ;; and MODE-ONLY in earlier versions. In either case we are interested in ;; whether it has the value `t'. (and (eq (ad-get-arg 0) t) ad-return-value ; A file-local mode was set. (so-long-handle-file-local-mode ad-return-value))) can be turned into (advice-add 'hack-local-variables :around #'so-long--hlv) (defun so-long--hlv (orig-fun &optional handle-mode &rest args) ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, ;; and MODE-ONLY in earlier versions. In either case we are interested in ;; whether it has the value `t'. (let ((retval (apply orig-fun handle-mode args))) (and (eq handle-mode t) retval ; A file-local mode was set. (so-long-handle-file-local-mode retval)))) BTW, while reading that code, I couldn't quite understand what this advice is about. I think a comment describing a use-case would be valuable. > Such changes will need to work in Emacs 25 at least, and ideally back > to 24.3 advice-add was added to Emacs-24.4 and it is available in GNU ELPA for earlier Emacsen, so it's OK. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-01-12 15:11 ` Stefan Monnier @ 2019-01-12 21:01 ` Stefan Monnier 2019-04-14 13:09 ` Phil Sainty 1 sibling, 0 replies; 33+ messages in thread From: Stefan Monnier @ 2019-01-12 21:01 UTC (permalink / raw) To: Phil Sainty; +Cc: emacs-devel >>> On the design side, I think you should merge `so-long` and >>> `so-long-mode` into a single function and make that function >>> a (buffer-local) minor-mode (i.e. not have any major mode, just use >>> fundamental-mode instead). Making it a minor mode rather than >>> a major-mode will also make it easier to remember the previous >>> major-mode without any need for a change-major-mode-hook hack. >> These changes are too significant, so I don't wish to do any of that. > I think they're just some minor shuffling of code. Also, I'm wondering which part you don't like. There are fundamentally two aspects in my suggestion: - merge the two - use a minor mode I think the most important for the user is the first: the difference between M-x so-long RET and M-x so-long-mode RET seems fairly subtle to me and I expect users will have trouble remembering which is which or even understand that they don't do the same. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-01-12 15:11 ` Stefan Monnier 2019-01-12 21:01 ` Stefan Monnier @ 2019-04-14 13:09 ` Phil Sainty 2019-04-14 15:14 ` Stefan Monnier 1 sibling, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-04-14 13:09 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel TL;DR: 1. Things I've changed: a) Per your suggestion, the automated behaviour is now enabled and disabled via a global minor mode, `global-so-long-mode'. b) The 'overrides-only action is now a buffer-local minor mode, which can be used/toggled by itself; so users now have a minor mode (`so-long-minor-mode') which will toggle the default performance mitigations. c) Switched from defadvice to nadvice. d) Improved / clarified documentation. 2. Things I'm not changing: a) `so-long-mode' remains a major mode. b) The `so-long' and `so-long-revert' commands remain as they were; dispatching to the action functions set in `so-long-action-alist'. As before, the latest code is here: https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip Responses in depth: On 13/01/19 4:11 AM, Stefan Monnier wrote: > The issue is consistency (a minor mode is a standard interface and > UI to enable&disable&test a feature) and ease of use (a global minor > mode can be enabled via Customize). > > Redefine so-long-enable as an obsolete function that just calls > (so-long-auto-mode 1). Ok, using the Customize interface to enable the library seemed like a good reason to do this. I've added `global-so-long-mode' which runs the enable/disable behaviour appropriately, with `so-long-enable' and `so-long-disable' redefined as suggested, and the documentation changed to refer to the global mode. > Side note: I mistakenly thought so-long was pretty much a brand new > package. How far back does it go? The initial release was in January 2016, and version 0.7.6 (being the latest release until the recent enhancements) was in July 2016. Until the recent work, the `so-long-mode' major mode was the only mitigation the library provided, and so all of the behaviours were tied to that. >>> Rather than defadvice, please use advice-add. > > E.g. > > (defadvice hack-local-variables (after so-long--file-local-mode disable) > "..." > ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, > ;; and MODE-ONLY in earlier versions. In either case we are interested in > ;; whether it has the value `t'. > (and (eq (ad-get-arg 0) t) > ad-return-value ; A file-local mode was set. > (so-long-handle-file-local-mode ad-return-value))) > > can be turned into > > (advice-add 'hack-local-variables :around #'so-long--hlv) > (defun so-long--hlv (orig-fun &optional handle-mode &rest args) > ;; The first arg to `hack-local-variables' is HANDLE-MODE since Emacs 26.1, > ;; and MODE-ONLY in earlier versions. In either case we are interested in > ;; whether it has the value `t'. > (let ((retval (apply orig-fun handle-mode args))) > (and (eq handle-mode t) > retval ; A file-local mode was set. > (so-long-handle-file-local-mode retval)))) This is changing 'after' advice into 'around' advice, which means that your version changes the return value, so that's definitely not equivalent. I've changed it to explicitly return retval. Can nadvice not do 'after' advice which knows the arguments? > (defadvice hack-local-variables (after so-long--file-local-mode disable) > > BTW, while reading that code, I couldn't quite understand what this > advice is about. I think a comment describing a use-case would be > valuable. That advice and `so-long-check-header-modes' each handle one of the ways in which a file-local 'mode' could be set. If the file does indeed have a file-local mode, then `so-long-handle-file-local-mode' is called. This is on the basis that file-local modes present a special-case -- when a file contains long lines yet a file-local mode was also set, it is reasonable to suspect that, despite the long lines, the mode is actually safe to use in practice (as if the mode was prohibitively slow with that file, the author who added the file-local mode would probably not have done so). That's guess-work as far as so-long is concerned of course, so `so-long-file-local-mode-function' is a user option which lets the user decide how those situations should be handled (with the default behaviour being fairly conservative). The "Files with a file-local 'mode'" section of the Commentary says much the same, but I'd not yet added that at the time you queried this. I think that ought to provide the missing context for readers (as otherwise I think all the advice is documented pretty clearly?) >> Such changes will need to work in Emacs 25 at least, and ideally >> back to 24.3 > > advice-add was added to Emacs-24.4 and it is available in GNU ELPA > for earlier Emacsen, so it's OK. So: Package-Requires: ((emacs "24.3") nadvice), yes? No -- experimentally that causes Emacs 26.1 to install nadvice from ELPA instead of deferring to the library it already has. Can I tell package.el that it's an ELPA requirement only if nadvice.el can't already be located? (Is that not what Package-Requires should mean by default?) For now I've bumped the required Emacs version to 24.4 instead. On 13/01/19 10:01 AM, Stefan Monnier wrote: >>> On the design side, I think you should merge `so-long` and >>> `so-long-mode` into a single function and make that function a >>> (buffer-local) minor-mode (i.e. not have any major mode, just use >>> fundamental-mode instead). Making it a minor mode rather than a >>> major-mode will also make it easier to remember the previous >>> major-mode without any need for a change-major-mode-hook hack. >> >> These changes are too significant, so I don't wish to do any of that. > > I think they're just some minor shuffling of code. > > Also, I'm wondering which part you don't like. It's partly that I've put a lot of effort into the way it is now, and the idea of reworking things at this stage is honestly painful. I'd already tried a few different approaches along the way to see what seemed to work well in practice, and heavily reworked some aspects in order to make it more extensible and easier to customize for the user; and spent a lot of time figuring out edge cases and backwards-compatibility issues with earlier versions of Emacs, sorting them all out, and polishing it all to a release state I was happy with, including working pretty hard on documenting everything. I'll concur that it seems more complicated than it might be; but it's flexible, and it works well. My gut says that the changes you're suggesting will inevitably entail a lot more of those time-consuming activities aside from any "minor shuffling of code" (and I don't think it's going to be as minor as that); so assuming it would also be me doing the rework, the notion holds no appeal. Moreover, I just don't think merging `so-long' and `so-long-mode' is the right thing to do. I do appreciate that your suggestion *sounds* like it would be clean and simple (and consequently an appealing improvement), but I don't think it actually provides the same functionality (or doing so winds up adding other new complications). Having a major mode is important to the usability. Even though I've added the ability to choose different 'actions', I've kept `so-long-mode' as the default on purpose, because I think it's important that when the automated behaviour kicks in, the reason is very much "in your face". When the user visits a file and everything looks wrong ("What's happened? Where is all my syntax highlighting?"), a major mode makes it VERY apparent what the cause is, and the explanation is an easy 'C-h m' away, with the needed documentation right at the top of the *Help* buffer, rather than buried somewhere within a long list of minor modes (which the user wouldn't necessarily otherwise realise they were looking for). There are some other side-benefits of having a major mode option as well, but I believe this default makes the effects more obvious, and consequently the library more usable. The major mode also means that for this default case I can safely bind the familiar 'C-c C-c' sequence to the revert function, which then makes it super easy for users to revert in false-positive cases; so it's hardly even a bother if that happens. > There are fundamentally two aspects in my suggestion: > - merge the two > - use a minor mode > > I think the most important for the user is the first: the difference > between M-x so-long RET and M-x so-long-mode RET seems fairly subtle > to me and I expect users will have trouble remembering which is > which or even understand that they don't do the same. This is a documentation problem, so I've addressed it as such. At the time it hadn't been sufficiently obvious to me that the two would be confused, because I knew the differences so well myself. >> There's backwards-compatibility with the earlier releases to >> consider (so-long-mode has always been a major mode); > > Then use another name for the minor mode and make so-long-mode be the > combination of fundamental-mode + so-long-minor-mode. If there's no actual major mode, then we lose the benefits of having this functionality available in the form of a major mode. Also, `so-long-mode' does very specific things, while `so-long' does whatever `so-long-action' tells it to; so doing the equivalent of the latter in fundamental-mode is not necessarily similar to the current major mode. >> I have a use-case for using it as a file-local 'mode' on one >> project, and I've had cause to suggest the same thing to other >> users. > > Minor modes can also be used on the "mode:" thingy, tho I don't > think that should make a big difference. They can be, but, to the best of my understanding, they shouldn't be. Using a minor mode as a file-local 'mode' is explicitly deprecated -- it's specifically stated that only major modes should be used as a 'mode' value, and minor modes should be enabled via 'eval'. The comments in `set-auto-mode' (also in `so-long-check-header-modes', which needed to duplicate that code) read: ;; Once we drop the deprecated feature where mode: is also allowed to ;; specify minor-modes (ie, there can be more than one "mode:"), we can ;; remove this section and just let (hack-local-variables t) handle it. I also think the behaviour when there's a single 'mode' value but it isn't a major mode can easily be confusing. It seems like something which might be working only by accident and might be brittle. Or to put it another way, the notion of a non-major mode that gets used as if it were a major mode is, to me, a more confusing prospect than any confusion you're trying to avoid by proposing that approach. (Tangentially, I've often thought that modes should have an easy way of being inspected and identified as major or minor, buffer-local or global, or globalized (controller or control-ee); and if that happened then it could sensibly be used to enforce the restriction of 'mode' to major modes, which would then be at odds with any hack to treat a minor mode as a major mode.) Regarding using a minor mode instead of `so-long'/`so-long-revert'... > The point of a minor mode is to provide a standard UI to enable > *and* disable something. Having now established that I'm keeping the major mode, I think that a minor mode which controls a major mode would also be a confusing situation. i.e. At that point we would have: global-so-long-mode calling a buffer-local minor mode which is (potentially) calling our major mode, so all three could be listed when the user typed C-h m. The minor mode would then need to remain active after that particular major mode change (but also *not* remain active if some other major mode change occurred). I know that's not what you had in mind (on account of your wanting to eliminate the major mode); but as I feel strongly about retaining the major mode, I also feel strongly that we don't want to create the above situation -- especially not on the basis of an attempt to reduce the potential for confusion. The `so-long' command is really a dispatcher -- it invokes an action, and that action can be anything, and it's simpler to keep it that way. The part that conceptually seemed like more of a candidate for a minor mode was the `overrides-only' action, which was essentially the same as `so-long-mode' but without the major mode. Originally I had dismissed the notion of that being a minor mode (on the basis that it could add confusion for no real benefit); but I've changed my mind and implemented it: `so-long-minor-mode' now exists as the minor-mode equivalent to `so-long-mode'. This possibly achieves some middle ground with your suggestions, as the user can, if they wish, invoke this minor mode directly (to much the same effect as setting `so-long-action' to the minor mode value, and calling `so-long'). -Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-04-14 13:09 ` Phil Sainty @ 2019-04-14 15:14 ` Stefan Monnier 2019-04-14 22:33 ` Phil Sainty 0 siblings, 1 reply; 33+ messages in thread From: Stefan Monnier @ 2019-04-14 15:14 UTC (permalink / raw) To: Phil Sainty; +Cc: emacs-devel >> (advice-add 'hack-local-variables :around #'so-long--hlv) >> (defun so-long--hlv (orig-fun &optional handle-mode &rest args) >> ;; The first arg to `hack-local-variables' is HANDLE-MODE since > Emacs 26.1, >> ;; and MODE-ONLY in earlier versions. In either case we are > interested in >> ;; whether it has the value `t'. >> (let ((retval (apply orig-fun handle-mode args))) >> (and (eq handle-mode t) >> retval ; A file-local mode was set. >> (so-long-handle-file-local-mode retval)))) > > This is changing 'after' advice into 'around' advice, which means that > your version changes the return value, so that's definitely not > equivalent. Oops, indeed, sorry. > I've changed it to explicitly return retval. Good, thanks. > Can nadvice not do 'after' advice which knows the arguments? The arguments, yes, but the just-computed return value, no. > The "Files with a file-local 'mode'" section of the Commentary says > much the same, but I'd not yet added that at the time you queried > this. I think that ought to provide the missing context for readers > (as otherwise I think all the advice is documented pretty clearly?) I must admit that I do a lot of hacking on packages I never use, so I like it when the code explains what it does without requiring the coder to have read the doc ;-) >>> Such changes will need to work in Emacs 25 at least, and ideally >>> back to 24.3 >> advice-add was added to Emacs-24.4 and it is available in GNU ELPA >> for earlier Emacsen, so it's OK. > So: Package-Requires: ((emacs "24.3") nadvice), yes? > No -- experimentally that causes Emacs 26.1 to install nadvice from > ELPA instead of deferring to the library it already has. IIUC that's a bug in Emacs-26.1 (and 26.2) because 26.[12] is not aware that it comes with nadvice-1.0. It really should have been fixed before Emacs-26.2, but I somehow forgot to push that commit and it lingered on a machine I only used again very recently :-( Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library 2019-04-14 15:14 ` Stefan Monnier @ 2019-04-14 22:33 ` Phil Sainty 2019-06-27 13:46 ` Performance degradation from long lines Phil Sainty 0 siblings, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-04-14 22:33 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel On 2019-04-15 03:14, Stefan Monnier wrote: >> So: Package-Requires: ((emacs "24.3") nadvice), yes? >> No -- experimentally that causes Emacs 26.1 to install nadvice from >> ELPA instead of deferring to the library it already has. > > IIUC that's a bug in Emacs-26.1 (and 26.2) because 26.[12] is not aware > that it comes with nadvice-1.0. > > It really should have been fixed before Emacs-26.2, but I somehow > forgot > to push that commit and it lingered on a machine I only used again > very recently :-( Ah, ok. I'll just stick with the new minimum version of 24.4, and side- step the ELPA issue. A single point-release increase in three years isn't so bad. -Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-04-14 22:33 ` Phil Sainty @ 2019-06-27 13:46 ` Phil Sainty 2019-07-06 14:18 ` Phil Sainty 0 siblings, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-06-27 13:46 UTC (permalink / raw) To: emacs-devel; +Cc: Eli Zaretskii, mithraeum, Stefan Monnier The origin/scratch/so-long branch (adding so-long.el) is at a release candidate stage, and I have rebased it over the current master branch. Since the last discussions I've added a test suite, which is passing on all the versions I've tested, being: 24.4 (the minimum supported version), 24.5, 25.3, 26.1, 26.2, and current master, and I've dealt to a variety of minor and backwards-compatibility issues in the process. As previously discussed, the ELPA package (for Emacs 24,25,26) can be built from this code. https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip also has the same code, just with a different directory structure for the test code). There will be some continuing work to eliminate the use of advice in Emacs 27 (e.g. via bug#35351), but I feel that the library is in a good state, and could potentially be merged at this point. -Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-06-27 13:46 ` Performance degradation from long lines Phil Sainty @ 2019-07-06 14:18 ` Phil Sainty 2019-07-13 8:07 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-07-06 14:18 UTC (permalink / raw) To: emacs-devel; +Cc: Eli Zaretskii, mithraeum, Stefan Monnier I'll merge scratch/so-long to master in another week or so if there's no objections. I'm keen to get additional input on some of the default config, but it'll be that much easier to get people to test and provide additional suggestions once it's been merged. -Phil On 28/06/19 1:46 AM, Phil Sainty wrote: > The origin/scratch/so-long branch (adding so-long.el) is at a release > candidate stage, and I have rebased it over the current master branch. > > Since the last discussions I've added a test suite, which is passing > on all the versions I've tested, being: 24.4 (the minimum supported > version), 24.5, 25.3, 26.1, 26.2, and current master, and I've dealt Now tested with 25.1 and 25.2 as well. > to a variety of minor and backwards-compatibility issues in the process. > > As previously discussed, the ELPA package (for Emacs 24,25,26) can be > built from this code. > > https://git.savannah.nongnu.org/cgit/so-long.git/plain/so-long.el?h=wip > also has the same code, just with a different directory structure for > the test code). > > There will be some continuing work to eliminate the use of advice in > Emacs 27 (e.g. via bug#35351), but I feel that the library is in a > good state, and could potentially be merged at this point. > > > -Phil > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-06 14:18 ` Phil Sainty @ 2019-07-13 8:07 ` Eli Zaretskii 2019-07-13 9:07 ` Stefan Kangas 2019-07-13 9:33 ` Phil Sainty 0 siblings, 2 replies; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 8:07 UTC (permalink / raw) To: Phil Sainty; +Cc: mithraeum, monnier, emacs-devel > From: Phil Sainty <psainty@orcon.net.nz> > Cc: Eli Zaretskii <eliz@gnu.org>, mithraeum <mithraeum@protonmail.com>, > Stefan Monnier <monnier@iro.umontreal.ca> > Date: Sun, 7 Jul 2019 02:18:17 +1200 > > I'll merge scratch/so-long to master in another week or so if there's > no objections. > > I'm keen to get additional input on some of the default config, but > it'll be that much easier to get people to test and provide additional > suggestions once it's been merged. Thanks for merging this. I made a few minor fixes to the docs. One comment I have is that disabling bidi-display-reordering should probably be removed from the defaults, because doing so puts the display engine in a state that is not being tested, and can cause inconsistencies and even bugs (because some portions of the code were written under the assumption that this variable is never nil). OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right by default when so-long-mode is turned on. Also, I don't understand why the defaults disable display-line-number-mode, it AFAIK does not slow down redisplay in any significant ways. Do you have any evidence it should be disabled in buffers with long lines? IME, truncate-lines sometimes makes display of long lines _faster_, so I'm not sure we should disable that by default. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 8:07 ` Eli Zaretskii @ 2019-07-13 9:07 ` Stefan Kangas 2019-07-13 9:51 ` Eli Zaretskii 2019-07-13 9:33 ` Phil Sainty 1 sibling, 1 reply; 33+ messages in thread From: Stefan Kangas @ 2019-07-13 9:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Phil Sainty, mithraeum, Stefan Monnier, emacs-devel Eli Zaretskii <eliz@gnu.org> writes: > One comment I have is that disabling bidi-display-reordering should > probably be removed from the defaults, because doing so puts the > display engine in a state that is not being tested, and can cause > inconsistencies and even bugs (because some portions of the code were > written under the assumption that this variable is never nil). I think some users are already setting bidi-display-reordering to nil. I stumbled into this the other day, for example: You can try and see if setting bidi-display-reordering to nil improves the speed for you. This removes one significant contributor to line scans, but sadly not the only one. https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow Should Emacs support that? Should it be added to its doc string that it could be dangerous? Thanks, Stefan Kangas ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 9:07 ` Stefan Kangas @ 2019-07-13 9:51 ` Eli Zaretskii 2019-07-13 10:23 ` Stefan Kangas 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 9:51 UTC (permalink / raw) To: Stefan Kangas; +Cc: psainty, mithraeum, monnier, emacs-devel > From: Stefan Kangas <stefan@marxist.se> > Date: Sat, 13 Jul 2019 11:07:08 +0200 > Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, > Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org > > Eli Zaretskii <eliz@gnu.org> writes: > > > One comment I have is that disabling bidi-display-reordering should > > probably be removed from the defaults, because doing so puts the > > display engine in a state that is not being tested, and can cause > > inconsistencies and even bugs (because some portions of the code were > > written under the assumption that this variable is never nil). > > I think some users are already setting bidi-display-reordering to nil. I > stumbled into this the other day, for example: They do that at their own risk. > Should Emacs support that? If someone wants to work on that, they should feel free. I won't. > Should it be added to its doc string that it could be dangerous? I'm okay with documenting that, thanks. Although the variable was supposed to be an internal on, but I guess that genie is out of the bottle long ago. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 9:51 ` Eli Zaretskii @ 2019-07-13 10:23 ` Stefan Kangas 2019-07-13 10:29 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Stefan Kangas @ 2019-07-13 10:23 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Phil Sainty, mithraeum, Stefan Monnier, emacs-devel [-- Attachment #1: Type: text/plain, Size: 374 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > > Should it be added to its doc string that it could be dangerous?> > I'm okay with documenting that, thanks. Although the variable was > supposed to be an internal on, but I guess that genie is out of the > bottle long ago. How about the attached patch? It's based on what you've said in this thread so far. Thanks, Stefan Kangas [-- Attachment #2: 0001-Add-warning-to-bidi-display-reordering-doc-string.patch --] [-- Type: text/x-patch, Size: 1621 bytes --] From cc3b8e1b0ecf7dca67246878f9374780685f0f5d Mon Sep 17 00:00:00 2001 From: Stefan Kangas <stefankangas@gmail.com> Date: Sat, 13 Jul 2019 12:11:19 +0200 Subject: [PATCH] Add warning to bidi-display-reordering doc string This explanation was given by Eli Zaretskii on emacs-devel. For discussion, see: https://lists.gnu.org/archive/html/emacs-devel/2019-07/msg00294.html * src/buffer.c (syms_of_buffer): Add warning to doc string of bidi-display-reordering to explain it could be dangerous in a production session. --- src/buffer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 209e29f0f1..9b978c131e 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -5659,8 +5659,12 @@ syms_of_buffer (void) DEFVAR_PER_BUFFER ("bidi-display-reordering", &BVAR (current_buffer, bidi_display_reordering), Qnil, - doc: /* Non-nil means reorder bidirectional text for display in the visual order. */); - + doc: /* Non-nil means reorder bidirectional text for display in the visual order. +WARNING: This variable is useful for debugging the display code, but +using it in a production session is dangerous, because its puts the +display engine in a state that is not being tested. It can cause +inconsistencies and even bugs (because some portions of the code were +written under the assumption that this variable is never nil). */); DEFVAR_PER_BUFFER ("bidi-paragraph-start-re", &BVAR (current_buffer, bidi_paragraph_start_re), Qnil, doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs. -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 10:23 ` Stefan Kangas @ 2019-07-13 10:29 ` Eli Zaretskii 2019-07-13 10:38 ` Stefan Kangas 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 10:29 UTC (permalink / raw) To: Stefan Kangas; +Cc: psainty, mithraeum, monnier, emacs-devel > From: Stefan Kangas <stefan@marxist.se> > Date: Sat, 13 Jul 2019 12:23:20 +0200 > Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, > Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org > > > I'm okay with documenting that, thanks. Although the variable was > > supposed to be an internal on, but I guess that genie is out of the > > bottle long ago. > > How about the attached patch? It's based on what you've said in this > thread so far. Well, I guess I said too much ;-) It should be enough to say something shorter like Setting this to nil is intended for use in debugging the display code. Don't set to nil in normal sessions, as that is not supported. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 10:29 ` Eli Zaretskii @ 2019-07-13 10:38 ` Stefan Kangas 2019-07-13 10:58 ` Phil Sainty 2019-07-13 11:23 ` Eli Zaretskii 0 siblings, 2 replies; 33+ messages in thread From: Stefan Kangas @ 2019-07-13 10:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Phil Sainty, mithraeum, Stefan Monnier, emacs-devel [-- Attachment #1: Type: text/plain, Size: 348 bytes --] Eli Zaretskii <eliz@gnu.org> writes: > Well, I guess I said too much ;-) > > It should be enough to say something shorter like > > Setting this to nil is intended for use in debugging the display > code. Don't set to nil in normal sessions, as that is not supported. OK, I've attached an updated patch with that text. Thanks, Stefan Kangas [-- Attachment #2: 0001-Add-warning-to-bidi-display-reordering-doc-string.patch --] [-- Type: text/x-patch, Size: 1408 bytes --] From cbe0a0fa166ed1a0704a40c8566a72740c37de4c Mon Sep 17 00:00:00 2001 From: Stefan Kangas <stefankangas@gmail.com> Date: Sat, 13 Jul 2019 12:11:19 +0200 Subject: [PATCH] Add warning to bidi-display-reordering doc string This explanation was given by Eli Zaretskii on emacs-devel. For discussion, see: https://lists.gnu.org/archive/html/emacs-devel/2019-07/msg00294.html * src/buffer.c (syms_of_buffer): Add warning to doc string of bidi-display-reordering to explain that it should only be used for debugging. --- src/buffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 209e29f0f1..859c09f29f 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -5659,8 +5659,9 @@ syms_of_buffer (void) DEFVAR_PER_BUFFER ("bidi-display-reordering", &BVAR (current_buffer, bidi_display_reordering), Qnil, - doc: /* Non-nil means reorder bidirectional text for display in the visual order. */); - + doc: /* Non-nil means reorder bidirectional text for display in the visual order. +Setting this to nil is intended for use in debugging the display code. +Don't set to nil in normal sessions, as that is not supported. */); DEFVAR_PER_BUFFER ("bidi-paragraph-start-re", &BVAR (current_buffer, bidi_paragraph_start_re), Qnil, doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs. -- 2.11.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 10:38 ` Stefan Kangas @ 2019-07-13 10:58 ` Phil Sainty 2019-07-13 11:23 ` Eli Zaretskii 2019-07-13 11:23 ` Eli Zaretskii 1 sibling, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-07-13 10:58 UTC (permalink / raw) To: Stefan Kangas, Eli Zaretskii; +Cc: mithraeum, Stefan Monnier, emacs-devel I think it would be good if that docstring also told users that setting bidi-paragraph-direction to 'left-to-right is what they should probably be doing instead? On 13/07/19 10:38 PM, Stefan Kangas wrote: > Eli Zaretskii <eliz@gnu.org> writes: > >> Well, I guess I said too much ;-) >> >> It should be enough to say something shorter like >> >> Setting this to nil is intended for use in debugging the display >> code. Don't set to nil in normal sessions, as that is not supported. > > OK, I've attached an updated patch with that text. > > Thanks, > Stefan Kangas > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 10:58 ` Phil Sainty @ 2019-07-13 11:23 ` Eli Zaretskii 0 siblings, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 11:23 UTC (permalink / raw) To: Phil Sainty; +Cc: mithraeum, stefan, monnier, emacs-devel > Cc: mithraeum@protonmail.com, Stefan Monnier <monnier@iro.umontreal.ca>, > emacs-devel@gnu.org > From: Phil Sainty <psainty@orcon.net.nz> > Date: Sat, 13 Jul 2019 22:58:51 +1200 > > I think it would be good if that docstring also told users that > setting bidi-paragraph-direction to 'left-to-right is what they > should probably be doing instead? Done, thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 10:38 ` Stefan Kangas 2019-07-13 10:58 ` Phil Sainty @ 2019-07-13 11:23 ` Eli Zaretskii 1 sibling, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 11:23 UTC (permalink / raw) To: Stefan Kangas; +Cc: psainty, mithraeum, monnier, emacs-devel > From: Stefan Kangas <stefan@marxist.se> > Date: Sat, 13 Jul 2019 12:38:44 +0200 > Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, > Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org > > > Setting this to nil is intended for use in debugging the display > > code. Don't set to nil in normal sessions, as that is not supported. > > OK, I've attached an updated patch with that text. Thanks, pushed to the emacs-26 branch. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 8:07 ` Eli Zaretskii 2019-07-13 9:07 ` Stefan Kangas @ 2019-07-13 9:33 ` Phil Sainty 2019-07-13 9:56 ` Eli Zaretskii 1 sibling, 1 reply; 33+ messages in thread From: Phil Sainty @ 2019-07-13 9:33 UTC (permalink / raw) To: Eli Zaretskii; +Cc: mithraeum, monnier, emacs-devel On 13/07/19 8:07 PM, Eli Zaretskii wrote: > One comment I have is that disabling bidi-display-reordering should > probably be removed from the defaults, because doing so puts the > display engine in a state that is not being tested, and can cause > inconsistencies and even bugs (because some portions of the code > were written under the assumption that this variable is never nil). > > OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right > by default when so-long-mode is turned on. That sounds good to me. I wasn't aware that nil was an invalid value for bidi-display-reordering. I would suggest that its docstring be updated in this regard. Currently the doc is just "Non-nil means reorder bidirectional text for display in the visual order." which to me implies that value of nil is ok. > Also, I don't understand why the defaults disable > display-line-number-mode, it AFAIK does not slow down redisplay in > any significant ways. Do you have any evidence it should be > disabled in buffers with long lines? No, I'd simply included it along with the older line-numbering minor modes. I believe I can see a *very* slight difference, depending on the state of display-line-number-mode, when moving around the visual lines in a ~1MB line; however it's not significant, so I don't object to removing it from the list. I'm not sure that there's a benefit to the end-user in having line- numbering enabled in such a file, though (which was the other reason I went ahead and added all of those modes). Some of the default minor modes are in the list because I felt the features they provided would be redundant in a long-lines situation (or redundant with font-lock-mode disabled), rather than because I'd done benchmarking and established that they were notable CPU hogs. I'd concluded that any mode or feature which scans buffer contents in order to highlight things would be reasonable to disable by default, and my general approach has been to err on the side of being overly conservative in order to provide maximum benefit in the really extreme cases. I'm certain that the default settings can be improved though, so I'm very happy for them to be discussed, and I hope that people will do some experimentation in their own configs and recommend changes and additions to the defaults, so that 27.1 ships with better coverage in general -- the current defaults really only reflect things which I've used personally. > IME, truncate-lines sometimes makes display of long lines _faster_, > so I'm not sure we should disable that by default. This should stay. The combination of truncate-lines being disabled and line-move-visual being enabled is a tremendous benefit when the user tries to move vertically from an extremely long line to the next line. With truncated lines, Emacs will have to scan to the end of the line (one of my test files is a 19M JSON file in a single line*), whereas with the settings I've used the user can happily move up and down in the early parts of the file with no problems at all. (*) You can find that file in this Stack Overflow question: https://emacs.stackexchange.com/q/598 (see the 'wget' command). -Phil ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 9:33 ` Phil Sainty @ 2019-07-13 9:56 ` Eli Zaretskii 2019-07-13 13:31 ` Stefan Monnier 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 9:56 UTC (permalink / raw) To: Phil Sainty; +Cc: mithraeum, monnier, emacs-devel > Cc: mithraeum@protonmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org > From: Phil Sainty <psainty@orcon.net.nz> > Date: Sat, 13 Jul 2019 21:33:19 +1200 > > On 13/07/19 8:07 PM, Eli Zaretskii wrote: > > One comment I have is that disabling bidi-display-reordering should > > probably be removed from the defaults, because doing so puts the > > display engine in a state that is not being tested, and can cause > > inconsistencies and even bugs (because some portions of the code > > were written under the assumption that this variable is never nil). > > > > OTOH, I'd suggest setting bidi-paragraph-direction to 'left-to-right > > by default when so-long-mode is turned on. > > That sounds good to me. I wasn't aware that nil was an invalid value > for bidi-display-reordering. It isn't invalid. It is useful for debugging the display code, but using it in production session is dangerous, because it causes the code to execute control-flow paths that aren't supported in general-purpose usage. > > Also, I don't understand why the defaults disable > > display-line-number-mode, it AFAIK does not slow down redisplay in > > any significant ways. Do you have any evidence it should be > > disabled in buffers with long lines? > > No, I'd simply included it along with the older line-numbering minor > modes. I believe I can see a *very* slight difference, depending on > the state of display-line-number-mode, when moving around the visual > lines in a ~1MB line; however it's not significant, so I don't object > to removing it from the list. In my measurements, the slow down is about 10% or less. > I'm not sure that there's a benefit to the end-user in having line- > numbering enabled in such a file, though (which was the other reason > I went ahead and added all of those modes). Log files is one important use case where line numbers might be of benefit. > > IME, truncate-lines sometimes makes display of long lines _faster_, > > so I'm not sure we should disable that by default. > > This should stay. The combination of truncate-lines being disabled > and line-move-visual being enabled is a tremendous benefit when the > user tries to move vertically from an extremely long line to the next > line. If it's the combination that matters, I suggest to mention that in the documentation (doc string, perhaps?), as I don't think this will be otherwise evident. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 9:56 ` Eli Zaretskii @ 2019-07-13 13:31 ` Stefan Monnier 2019-07-13 13:43 ` Stefan Kangas 2019-07-13 14:14 ` Eli Zaretskii 0 siblings, 2 replies; 33+ messages in thread From: Stefan Monnier @ 2019-07-13 13:31 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Phil Sainty, mithraeum, emacs-devel >> That sounds good to me. I wasn't aware that nil was an invalid value >> for bidi-display-reordering. > > It isn't invalid. It is useful for debugging the display code, but > using it in production session is dangerous, because it causes the > code to execute control-flow paths that aren't supported in > general-purpose usage. How 'bout we rename it to clarify it. The new name could include "debug", "internal" and/or "--". Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 13:31 ` Stefan Monnier @ 2019-07-13 13:43 ` Stefan Kangas 2019-07-13 14:14 ` Eli Zaretskii 1 sibling, 0 replies; 33+ messages in thread From: Stefan Kangas @ 2019-07-13 13:43 UTC (permalink / raw) To: Stefan Monnier; +Cc: Phil Sainty, Eli Zaretskii, mithraeum, emacs-devel Stefan Monnier <monnier@iro.umontreal.ca> writes: > >> That sounds good to me. I wasn't aware that nil was an invalid value > >> for bidi-display-reordering. > > > > It isn't invalid. It is useful for debugging the display code, but > > using it in production session is dangerous, because it causes the > > code to execute control-flow paths that aren't supported in > > general-purpose usage. > > How 'bout we rename it to clarify it. > The new name could include "debug", "internal" and/or "--". It should probably be "-internal", according to (info "(elisp)Tips for Defining") ‘...-internal’ The variable is intended for internal use and is defined in C code. (Emacs code contributed before 2018 may follow other conventions, which are being phased out.) Best regards, Stefan Kangas ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 13:31 ` Stefan Monnier 2019-07-13 13:43 ` Stefan Kangas @ 2019-07-13 14:14 ` Eli Zaretskii 2019-07-13 14:17 ` Stefan Monnier 1 sibling, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 14:14 UTC (permalink / raw) To: Stefan Monnier; +Cc: psainty, mithraeum, emacs-devel > From: Stefan Monnier <monnier@iro.umontreal.ca> > Date: Sat, 13 Jul 2019 09:31:02 -0400 > Cc: Phil Sainty <psainty@orcon.net.nz>, mithraeum@protonmail.com, > emacs-devel@gnu.org > > >> That sounds good to me. I wasn't aware that nil was an invalid value > >> for bidi-display-reordering. > > > > It isn't invalid. It is useful for debugging the display code, but > > using it in production session is dangerous, because it causes the > > code to execute control-flow paths that aren't supported in > > general-purpose usage. > > How 'bout we rename it to clarify it. > The new name could include "debug", "internal" and/or "--". And then introduce an alias or it, and maintain those for years? Doesn't sound worth the hassle. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 14:14 ` Eli Zaretskii @ 2019-07-13 14:17 ` Stefan Monnier 2019-07-13 18:17 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Stefan Monnier @ 2019-07-13 14:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: psainty, mithraeum, emacs-devel > And then introduce an alias for it, I was thinking of *not* having an alias for it. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 14:17 ` Stefan Monnier @ 2019-07-13 18:17 ` Eli Zaretskii 2019-07-13 22:22 ` Stefan Monnier 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-13 18:17 UTC (permalink / raw) To: Stefan Monnier; +Cc: psainty, mithraeum, emacs-devel > From: Stefan Monnier <monnier@iro.umontreal.ca> > Date: Sat, 13 Jul 2019 10:17:18 -0400 > Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org > > > And then introduce an alias for it, > > I was thinking of *not* having an alias for it. I don't see how we could avoid that, this variable is quite old and is known too widely to simply remove it. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 18:17 ` Eli Zaretskii @ 2019-07-13 22:22 ` Stefan Monnier 2019-07-14 5:39 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Stefan Monnier @ 2019-07-13 22:22 UTC (permalink / raw) To: Eli Zaretskii; +Cc: psainty, mithraeum, emacs-devel >> I was thinking of *not* having an alias for it. > I don't see how we could avoid that, Just like that: we rename it and announce the change in etc/NEWS. > this variable is quite old and is known too widely to simply remove it. I think the risk is very low. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-13 22:22 ` Stefan Monnier @ 2019-07-14 5:39 ` Eli Zaretskii 2019-07-15 13:12 ` Dmitry Gutov 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-14 5:39 UTC (permalink / raw) To: Stefan Monnier; +Cc: psainty, mithraeum, emacs-devel > From: Stefan Monnier <monnier@iro.umontreal.ca> > Date: Sat, 13 Jul 2019 18:22:43 -0400 > Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org > > >> I was thinking of *not* having an alias for it. > > I don't see how we could avoid that, > > Just like that: we rename it and announce the change in etc/NEWS. Sorry, that would be burying our head in the sand. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-14 5:39 ` Eli Zaretskii @ 2019-07-15 13:12 ` Dmitry Gutov 2019-07-18 6:30 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Dmitry Gutov @ 2019-07-15 13:12 UTC (permalink / raw) To: Eli Zaretskii, Stefan Monnier; +Cc: psainty, mithraeum, emacs-devel On 14.07.2019 8:39, Eli Zaretskii wrote: >> Just like that: we rename it and announce the change in etc/NEWS. > > Sorry, that would be burying our head in the sand. If the variable is only used out there for performance purposes, or temporarily for debugging, I don't think it would lead to any significant breakage, even if the former name suddenly stops working. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-15 13:12 ` Dmitry Gutov @ 2019-07-18 6:30 ` Eli Zaretskii 2019-07-18 14:48 ` Stefan Monnier 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2019-07-18 6:30 UTC (permalink / raw) To: Dmitry Gutov; +Cc: psainty, mithraeum, monnier, emacs-devel > Cc: psainty@orcon.net.nz, mithraeum@protonmail.com, emacs-devel@gnu.org > From: Dmitry Gutov <dgutov@yandex.ru> > Date: Mon, 15 Jul 2019 16:12:26 +0300 > > On 14.07.2019 8:39, Eli Zaretskii wrote: > > >> Just like that: we rename it and announce the change in etc/NEWS. > > > > Sorry, that would be burying our head in the sand. > > If the variable is only used out there for performance purposes, or > temporarily for debugging, I don't think it would lead to any > significant breakage, even if the former name suddenly stops working. You are missing the point, I think: this variable's name is known widely enough to make it a de-facto public interface. Changing its name without an alias will cause breakage to many people's configurations. However I dislike use of this variable in people's configurations, I cannot break them all just because I dislike that. IOW, it's a question of being fair to our users, even if they do something we don't endorse. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-18 6:30 ` Eli Zaretskii @ 2019-07-18 14:48 ` Stefan Monnier 2019-07-18 17:11 ` Clément Pit-Claudel 2019-07-18 18:33 ` Andy Moreton 0 siblings, 2 replies; 33+ messages in thread From: Stefan Monnier @ 2019-07-18 14:48 UTC (permalink / raw) To: Eli Zaretskii; +Cc: psainty, emacs-devel, mithraeum, Dmitry Gutov > You are missing the point, I think: this variable's name is known > widely enough to make it a de-facto public interface. Changing its > name without an alias will cause breakage to many people's > configurations. However I dislike use of this variable in people's > configurations, I cannot break them all just because I dislike that. I think we all understand that. But some of us think the breakage will be minimal. My cursory search through github seemed to indicate that it's very rarely usedin Elisp packages and the few times it's used it comes with a comment along the lines of "disable bidi for a marginal performance gain". As for such settings in people's .emacs, my educated guess is that they date back to Emacs-23's introduction to work around problems in specific modes that have since solved those problems (typically by setting bidi-paragraph-direction). IOW, any noticeable breakage that might result likely points to a *real* bug. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-18 14:48 ` Stefan Monnier @ 2019-07-18 17:11 ` Clément Pit-Claudel 2019-07-18 18:11 ` Stefan Monnier 2019-07-18 18:33 ` Andy Moreton 1 sibling, 1 reply; 33+ messages in thread From: Clément Pit-Claudel @ 2019-07-18 17:11 UTC (permalink / raw) To: emacs-devel On 2019-07-18 10:48, Stefan Monnier wrote: > As for such settings in people's .emacs, my educated guess is that they > date back to Emacs-23's introduction to work around problems in specific > modes that have since solved those problems (typically by setting > bidi-paragraph-direction). It's also suggested in the top-voted answer on how to make Emacs faster with long lines on emacs.stackexchange (https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow), so I'd expect many people to have it in their config for that reason. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-18 17:11 ` Clément Pit-Claudel @ 2019-07-18 18:11 ` Stefan Monnier 0 siblings, 0 replies; 33+ messages in thread From: Stefan Monnier @ 2019-07-18 18:11 UTC (permalink / raw) To: Clément Pit-Claudel; +Cc: emacs-devel >> As for such settings in people's .emacs, my educated guess is that they >> date back to Emacs-23's introduction to work around problems in specific >> modes that have since solved those problems (typically by setting >> bidi-paragraph-direction). > > It's also suggested in the top-voted answer on how to make Emacs faster with > long lines on emacs.stackexchange > (https://emacs.stackexchange.com/questions/598/how-do-i-prevent-extremely-long-lines-making-emacs-slow), > so I'd expect many people to have it in their config for that reason. Yes, but making it ineffective won't break their config. It will merely make long-lines a bit slower in some cases. And we can point to so-long for a better solution. Nothing problematic, IMO. Stefan ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Performance degradation from long lines 2019-07-18 14:48 ` Stefan Monnier 2019-07-18 17:11 ` Clément Pit-Claudel @ 2019-07-18 18:33 ` Andy Moreton 1 sibling, 0 replies; 33+ messages in thread From: Andy Moreton @ 2019-07-18 18:33 UTC (permalink / raw) To: emacs-devel On Thu 18 Jul 2019, Stefan Monnier wrote: >> You are missing the point, I think: this variable's name is known >> widely enough to make it a de-facto public interface. Changing its >> name without an alias will cause breakage to many people's >> configurations. However I dislike use of this variable in people's >> configurations, I cannot break them all just because I dislike that. > > I think we all understand that. But some of us think the breakage will > be minimal. It is perfectly reasonable to deprecate the variable. However it is better to show the user how to fix the problem in their code than just break previously working code (for some definition of working). How about renaming all occurences of `bidi-display-reordering' to `debug--bidi-display-reordering' or similar, and then add something like: (make-obsolete-variable 'bidi-display-reordering "not used. To disable reordering, use `(setq bidi-paragraph-direction 'left-to-right)' instead." "27.1") That would less hostile to users than immediate removal. AndyM ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2019-07-18 18:33 UTC | newest] Thread overview: 33+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20190107065207.21793.53271@vcs0.savannah.gnu.org> [not found] ` <20190107065208.BA36C21736@vcs0.savannah.gnu.org> 2019-01-10 18:07 ` [Emacs-diffs] scratch/so-long 7273fb2: Add so-long library Stefan Monnier 2019-01-12 2:20 ` Phil Sainty 2019-01-12 15:11 ` Stefan Monnier 2019-01-12 21:01 ` Stefan Monnier 2019-04-14 13:09 ` Phil Sainty 2019-04-14 15:14 ` Stefan Monnier 2019-04-14 22:33 ` Phil Sainty 2019-06-27 13:46 ` Performance degradation from long lines Phil Sainty 2019-07-06 14:18 ` Phil Sainty 2019-07-13 8:07 ` Eli Zaretskii 2019-07-13 9:07 ` Stefan Kangas 2019-07-13 9:51 ` Eli Zaretskii 2019-07-13 10:23 ` Stefan Kangas 2019-07-13 10:29 ` Eli Zaretskii 2019-07-13 10:38 ` Stefan Kangas 2019-07-13 10:58 ` Phil Sainty 2019-07-13 11:23 ` Eli Zaretskii 2019-07-13 11:23 ` Eli Zaretskii 2019-07-13 9:33 ` Phil Sainty 2019-07-13 9:56 ` Eli Zaretskii 2019-07-13 13:31 ` Stefan Monnier 2019-07-13 13:43 ` Stefan Kangas 2019-07-13 14:14 ` Eli Zaretskii 2019-07-13 14:17 ` Stefan Monnier 2019-07-13 18:17 ` Eli Zaretskii 2019-07-13 22:22 ` Stefan Monnier 2019-07-14 5:39 ` Eli Zaretskii 2019-07-15 13:12 ` Dmitry Gutov 2019-07-18 6:30 ` Eli Zaretskii 2019-07-18 14:48 ` Stefan Monnier 2019-07-18 17:11 ` Clément Pit-Claudel 2019-07-18 18:11 ` Stefan Monnier 2019-07-18 18:33 ` Andy Moreton
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).