From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Uday S Reddy Newsgroups: gmane.emacs.help Subject: RE: `save-excursion' defeated by `set-buffer' Date: Mon, 14 Mar 2011 23:20:58 +0000 Message-ID: <19838.41690.515000.197951@gargle.gargle.HOWL> References: <4D792D16.1080900@easy-emacs.de> <83y64ksoik.fsf@gnu.org> <87aah0j7lf.fsf@rapttech.com.au> <19836.48264.656000.373465@gargle.gargle.HOWL> <19838.9290.109000.165849@gargle.gargle.HOWL> <5F174275B43A467AA8FA2A1928774136@us.oracle.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1300144917 5204 80.91.229.12 (14 Mar 2011 23:21:57 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 14 Mar 2011 23:21:57 +0000 (UTC) Cc: 'Tim X' , 'Uday S Reddy' , help-gnu-emacs@gnu.org To: "Drew Adams" Original-X-From: help-gnu-emacs-bounces+geh-help-gnu-emacs=m.gmane.org@gnu.org Tue Mar 15 00:21:48 2011 Return-path: Envelope-to: geh-help-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1PzH50-0005yi-4p for geh-help-gnu-emacs@m.gmane.org; Tue, 15 Mar 2011 00:21:47 +0100 Original-Received: from localhost ([127.0.0.1]:42279 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PzH4x-0003YV-Lk for geh-help-gnu-emacs@m.gmane.org; Mon, 14 Mar 2011 19:21:35 -0400 Original-Received: from [140.186.70.92] (port=40391 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PzH4Y-0003X0-0u for help-gnu-emacs@gnu.org; Mon, 14 Mar 2011 19:21:11 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PzH4W-0005LC-BJ for help-gnu-emacs@gnu.org; Mon, 14 Mar 2011 19:21:09 -0400 Original-Received: from sun61.bham.ac.uk ([147.188.128.150]:64011) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PzH4W-0005Kq-0c for help-gnu-emacs@gnu.org; Mon, 14 Mar 2011 19:21:08 -0400 Original-Received: from [147.188.128.127] (helo=bham.ac.uk) by sun61.bham.ac.uk with esmtp (Exim 4.72) (envelope-from ) id 1PzH4U-00020F-N8; Mon, 14 Mar 2011 23:21:06 +0000 Original-Received: from mx1.cs.bham.ac.uk ([147.188.192.53]) by bham.ac.uk with esmtp (Exim 4.72) (envelope-from ) id 1PzH4U-0006fB-DE; Mon, 14 Mar 2011 23:21:06 +0000 Original-Received: from preston.cs.bham.ac.uk ([147.188.193.17] helo=MARUTI.cs.bham.ac.uk) by mx1.cs.bham.ac.uk with esmtp (Exim 4.51) id 1PzH4S-0003J1-SM; Mon, 14 Mar 2011 23:21:05 +0000 In-Reply-To: <5F174275B43A467AA8FA2A1928774136@us.oracle.com> X-Mailer: VM 8.1.93a under 23.2.94.1 (i386-mingw-nt5.1.2600) X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 147.188.128.150 X-BeenThere: help-gnu-emacs@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Users list for the GNU Emacs text editor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: help-gnu-emacs-bounces+geh-help-gnu-emacs=m.gmane.org@gnu.org Errors-To: help-gnu-emacs-bounces+geh-help-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.help:80114 Archived-At: Drew Adams writes: > Though you did not state this as part of your scenario, since you > have mentioned it otherwise may I add the assumption that this > hypothetical code came from an existing application written by a > good programmer, and that that code generally works well and has > done so for quite some time? No, it wasn't from an actual application. I just made it up. And, it isn't good code either. To make it good code, one needs another save-excursion to protect the point movement in buffer A, as below: (defun find-something () (.... (save-excursion (set-buffer A) (save-excursion (goto-char y) ...)))) The outer save-excursion essentially plays the role of save-current-buffer and it is mostly harmless when used in that way. The second save-excursion is needed to reverse the excursion of the point to y. Without the second save-excursion, I would consider the code buggy. Once that bug is fixed, the remaining save-excursions (the ones at the top-level in `find-something' as well as `my-beautiful-function') can be safely converted to save-current-buffer. And, the code will work fine. This is what I mean by tracking down the bugs covered up by redundant save-excursions. The clean-up procedure involves: - Converting a few save-excursions flagged up by the compiler to save-current-buffer. - Testing to see if those conversions break anything. - If they do, then hunt down the unprotected excursions that have been exposed in the process and wrap them in save-excursions. And repeat. It is painstaking work. You might wonder why do this at all. Why not just leave the code as it is since it is working fine? That is a judgement call for you as the developer. If it is important code and you expect it to get used for a long time hence, it pays to clean it up, because you never know when those unprotected excursions will come back to bite you. > IOW, I think you are trying to point out that code that has worked > well can nevertheless be fragile. So let's assume that with no > changes the code works, and the person who wrote it was no fool - > s?he wrote what s?he intended and needed. If that code was found in an actual application, then the only thing that can be said in defence of the original developer is that it must have been an oversight. In the pre-20.1 elisp, save-excursion was the only primitive available to restore the current buffer, which had the unfortunate effect of covering up all the unprotected excursions. So, s?he didn't notice the problem. If s?he deliberately wrote such code, then we should probably dump it and go find something else to work on. > Given the assumptions that I added (fairly, I hope), I would say > that presumably the `save-excursion' is there NOT ONLY to restore > which buffer was current but also restore point and mark in that > buffer. That's what `save-excursion' DOES, and there is no reason > not to suppose that the original author did not intend that. No way! If the original author deliberately used the outer save-excursion to cover up unprotected excursions, then the code is really bad. Let me amplify that point because this seems to be the crux of the discussion: (save-excursion (set-buffer B) .... (setq a (find-something)) ...) The outer save-excursion is restoring the current buffer's point and mark. There is nothing visible in the body that does anything to the current buffer's point and mark. So, the ONLY reason the outer save-excursion needs to restore the current buffer's point and mark is the expectation that `find-something' is going to do something cheeky. How can you trust such a programmer? > IOW, given inherited code that seems to work, assume as a start that > it works _because_ of the code that is there. Do not assume that > the code can just be altered willy-nilly, replacing some > functionality by a subset that does not do the whole job. That > might be the case, and perhaps the inheritor can improve the code, > but it would be a mistake to just assume that things can be changed > willy-nilly. If the code doesn't allow me to alter it "willy-nilly" then it isn't good code. Of course, I would need to understand its function, the data structures and invariants. But, assuming I have understood all that, I should be able to look at pieces of code locally and modify them. > IOW, again, do NOT assume that the `save-excursion' is intending to > only `save-current-buffer'. Such an assumption is unwarranted based > only on a glance at the `m-b-f' code - after all, it does call other > code. I think I have already made my disagrement plain. It is a bad idea for `my-beautiful-function' to be negating the other code's point movements at the top level. If it is clear from the way the "other code" is put together that its purpose is to change buffer and move point, and m-b-f is deliberately calling that code to achieve those effects, then why is it reversing all those changes via a save-excursion? That doesn't make much sense. Either - the other code is being called for its effects, in which case we have no need to reverse them, or - the other code is being called for some other results, in which case it should be cleaning up after itself. Slapping a save-excursion at the top-level, which deceptively looks like a save-current-buffer, to do a brute force clean-up after the fact is not a very clever way of arranging things. > On what basis could you presume anything different? You seem to be > so convinced (unlike Stefan, BTW) that `save-excursion' is evil that > you assume that its point-saving behavior is not needed (in code > that works). No, I didn't say that. `save-excursion' is definitely used to save the point and mark. But when used for that purpose, it is placed where the point or mark are being moved. If it is used to save the buffer, it is placed in front of a set-buffer. I have never really seen it being used to do both. If I have to imagine how it could be used to do both, it would be something like this: (save-excursion ... (goto-char x) ... (set-buffer B) (save-excursion ....)) A second save-excursion would be needed after switching to B, because whatever is done afterwards is *not* going to be protected by the original save-excursion. > > What makes the buffer A special? Why is it that you should > > always restore the point in the current buffer and leave all the other > > buffers to their fate? > > That is what `save-excursion' does. If you want something > different, then code something different. By hypothesis, the > original programmer used `save-excursion', and knew what s?he was > doing. Don't ask me why s?he felt that A needed restoring and not > other buffers. But s?he did (by hypothesis), or s?he wouldn't have > used `save-excursion'. We shouldn't call `save-excursion' willy-nilly just because it exists. If the buffer A is indeed special and we should restore only its excursions but not those of other buffers, then the right place to protect them is wherever the excursions are happening, not at the top-level. Morever, what if that is not what we want? What if we want to protect the excursions in all the buffers, not only the current-buffer of `m-b-f'? The top-level `save-excursion' is not going to help you achieve that. What do you do then? On a closer look, this kind of programming doesn't look well-thought out at all. It seems quite arbitrary. > Let me repeat: `save-excursion' is _not_ `save-current-buffer'. You > substitute the latter for the former willy-nilly at your own peril. I know. I was writing to Stefan earlier today saying exactly that. `save-excursion' should be replaced by `save-current-buffer' only after ensuring that all excursions are properly protected. I have described my clean-up procedure earlier in the message. > Dunno what to say, Uday, without repeating myself more. > `save-excursion' saves and restores point in only one buffer. If > that is not the intention then don't use it. What you call > "covering up" is exactly the ignoring of temporary point movements > (for _one_ buffer). "Only one buffer" is a mistaken assumption. Typically, in this kind of code, there would be save-excursions all over the place. Whenever point movements happen, they would be under a thick stack of save-excursions for all kinds of buffers. The buffer you are doing excursions in will typically end up being in that stack accidentaly and so its excursion will get cancelled before the whole process ends. Somehow magically the code works, even though you don't know how. When you tweak the code a little, some buffer might get eliminated from the stack and, all of a sudden, its excursions will get exposed to the user. "No problem!" says the hacker. Slap on another save-excursion, and the code starts to work magically again. > > If there is any other conceivable reason why `save-excursion' should > > be used with `set-buffer', then please show me! > > Please read the original thread if you don't get it by now. > > What if you need to save and later restore point in a buffer, Uday, what do > _you_ do? What do _I_ do? I never need to save and restore the point in a buffer. I always place my save-excursions next to the excursions. Only those point movements that are supposed to be the inteded effects of the operations will be outside any save-excursion. If I make any mistakes in doing this, I go find them and fix them. So, I don't need any save-excursions at the top-level. The code that I maintain works exactly in that way. And that is what the Elisp manual says you should do as well: It is often useful to move point "temporarily" within a localized portion of the program, or to switch buffers temporarily. This is called an "excursion", and it is done with the `save-excursion' special form. Cheers, Uday