From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Karl Fogel Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v Date: Wed, 19 Nov 2008 02:50:44 -0500 Message-ID: <87y6zggnmz.fsf@red-bean.com> References: Reply-To: Karl Fogel NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1227081064 1533 80.91.229.12 (19 Nov 2008 07:51:04 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 19 Nov 2008 07:51:04 +0000 (UTC) Cc: emacs-devel@gnu.org To: Chong Yidong Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Nov 19 08:52:05 2008 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1L2hr1-0007y7-LH for ged-emacs-devel@m.gmane.org; Wed, 19 Nov 2008 08:52:03 +0100 Original-Received: from localhost ([127.0.0.1]:41782 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2hpt-0001av-3y for ged-emacs-devel@m.gmane.org; Wed, 19 Nov 2008 02:50:53 -0500 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2hpo-0001Zx-0I for emacs-devel@gnu.org; Wed, 19 Nov 2008 02:50:48 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2hpn-0001Za-6r for emacs-devel@gnu.org; Wed, 19 Nov 2008 02:50:47 -0500 Original-Received: from [199.232.76.173] (port=55078 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2hpn-0001ZV-13 for emacs-devel@gnu.org; Wed, 19 Nov 2008 02:50:47 -0500 Original-Received: from mx20.gnu.org ([199.232.41.8]:46259) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1L2hpm-0000bF-Hf for emacs-devel@gnu.org; Wed, 19 Nov 2008 02:50:46 -0500 Original-Received: from sanpietro.red-bean.com ([66.146.193.61]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L2hpl-0002wn-Oj for emacs-devel@gnu.org; Wed, 19 Nov 2008 02:50:45 -0500 Original-Received: from localhost ([127.0.0.1]:41280 helo=floss) by sanpietro.red-bean.com with esmtp (Exim 4.69) (envelope-from ) id 1L2hpk-0006fl-WE; Wed, 19 Nov 2008 01:50:45 -0600 In-Reply-To: (Chong Yidong's message of "Wed, 29 Oct 2008 18:22:13 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.0.60 (gnu/linux) X-detected-kernel: by mx20.gnu.org: Linux 2.6 (newer, 3) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:105809 Archived-At: Regarding this change: Chong Yidong writes: > [[[ > (bookmark-get-bookmark-record): Signal error for invalid bookmark. > ]]] > > Index: bookmark.el > =================================================================== > RCS file: /sources/emacs/emacs/lisp/bookmark.el,v > retrieving revision 1.117 > retrieving revision 1.118 > diff -u -b -r1.117 -r1.118 > --- bookmark.el 29 Oct 2008 17:42:49 -0000 1.117 > +++ bookmark.el 29 Oct 2008 18:22:12 -0000 1.118 > @@ -330,7 +330,8 @@ > (defun bookmark-get-bookmark-record (bookmark) > "Return the guts of the entry for BOOKMARK in `bookmark-alist'. > That is, all information but the name." > - (let ((alist (cdr (bookmark-get-bookmark bookmark)))) > + (let ((alist (cdr (or (bookmark-get-bookmark bookmark) > + (error "Invalid bookmark %s" bookmark))))) > ;; The bookmark objects can either look like (NAME ALIST) or > ;; (NAME . ALIST), so we have to distinguish the two here. > (if (and (null (cdr alist)) (consp (caar alist))) Seeing this change made me ask: "Why doesn't `bookmark-get-bookmark' just return error itself, if no such bookmark?" Answer: "Because some callers might expect it to return nil in that case, rather than error." (Even though it is not documented to do either -- its doc string is silent on this question!) I think there are only two callers who would care, though. In `bookmark-store', we have this: (if (and (bookmark-get-bookmark stripped-name) (not no-overwrite)) ;; already existing bookmark under that name and ;; no prefix arg means just overwrite old bookmark ;; Use the new (NAME . ALIST) format. (setcdr (bookmark-get-bookmark stripped-name) alist) ;; otherwise just cons it onto the front (either the bookmark ;; doesn't exist already, or there is no prefix arg. In either ;; case, we want the new bookmark consed onto the alist...) (push (cons stripped-name alist) bookmark-alist)) And `bookmark-delete', this: (or (bookmark-get-bookmark bookmark-current-bookmark) (setq bookmark-current-bookmark nil))) I'm tempted to add an optional NOERROR parameter to `bookmark-get-bookmark'. That way we'd many more error checks for free, and could remove the special-case error check added in Chong Yidong's change above. Any objections to that? As an aside: it's very difficult to review & respond to commits to Emacs, because the ChangeLog entry arrives in a separate email from the diff. If the change package were kept intact (i.e., arrived as one email, the way every other project in the universe does it), we'd probably get more review here. -Karl