From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#5021: avl-tree.el enhancements Date: Mon, 23 Nov 2009 09:58:08 -0500 Message-ID: References: <20091123000123.GA7647@c3po> <20091123094431.GA31081@c3po> Reply-To: Stefan Monnier , 5021@emacsbugs.donarmstrong.com NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1258989070 843 80.91.229.12 (23 Nov 2009 15:11:10 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 23 Nov 2009 15:11:10 +0000 (UTC) Cc: bug-gnu-emacs@gnu.org To: Toby Cubitt Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Nov 23 16:11:02 2009 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1NCaZB-0006Ol-DF for geb-bug-gnu-emacs@m.gmane.org; Mon, 23 Nov 2009 16:11:02 +0100 Original-Received: from localhost ([127.0.0.1]:47818 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NCaZA-0005dE-8y for geb-bug-gnu-emacs@m.gmane.org; Mon, 23 Nov 2009 10:11:00 -0500 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NCaVm-0001ID-2U for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 10:07:30 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NCaVh-0001Em-8O for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 10:07:29 -0500 Original-Received: from [199.232.76.173] (port=39787 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NCaVg-0001ET-Qr for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 10:07:24 -0500 Original-Received: from rzlab.ucr.edu ([138.23.92.77]:53833) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NCaVg-0000Mj-76 for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 10:07:24 -0500 Original-Received: from rzlab.ucr.edu (rzlab.ucr.edu [127.0.0.1]) by rzlab.ucr.edu (8.14.3/8.14.3/Debian-5) with ESMTP id nANF7Lmu020632; Mon, 23 Nov 2009 07:07:21 -0800 Original-Received: (from debbugs@localhost) by rzlab.ucr.edu (8.14.3/8.14.3/Submit) id nANF557E019921; Mon, 23 Nov 2009 07:05:05 -0800 Resent-Date: Mon, 23 Nov 2009 07:05:05 -0800 X-Loop: owner@emacsbugs.donarmstrong.com Resent-From: Stefan Monnier Resent-To: bug-submit-list@donarmstrong.com Resent-CC: Emacs Bugs 2Resent-Date: Mon, 23 Nov 2009 15:05:05 +0000 Resent-Message-ID: Resent-Sender: owner@emacsbugs.donarmstrong.com X-Emacs-PR-Message: report 5021 X-Emacs-PR-Package: emacs X-Emacs-PR-Keywords: Original-Received: via spool by submit@emacsbugs.donarmstrong.com id=B.125898829918999 (code B ref -1); Mon, 23 Nov 2009 15:05:05 +0000 Original-Received: (at submit) by emacsbugs.donarmstrong.com; 23 Nov 2009 14:58:19 +0000 X-Spam-Bayes: score:0.5 Bayes not run. spammytokens:Tokens not available. hammytokens:Tokens not available. Original-Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) by rzlab.ucr.edu (8.14.3/8.14.3/Debian-5) with ESMTP id nANEwGWP018996 for ; Mon, 23 Nov 2009 06:58:18 -0800 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NCaMq-0004Zl-M9 for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 09:58:16 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NCaMl-0004YX-UO for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 09:58:16 -0500 Original-Received: from [199.232.76.173] (port=33238 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NCaMl-0004YJ-Hv for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 09:58:11 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.183]:53065 helo=ironport2-out.pppoe.ca) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NCaMj-000751-Fx for bug-gnu-emacs@gnu.org; Mon, 23 Nov 2009 09:58:09 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqsEANkxCkvO+IIa/2dsb2JhbACBTdQfhDwEgxmGaQ X-IronPort-AV: E=Sophos;i="4.47,272,1257138000"; d="scan'208";a="49846666" Original-Received: from 206-248-130-26.dsl.teksavvy.com (HELO pastel.home) ([206.248.130.26]) by ironport2-out.pppoe.ca with ESMTP; 23 Nov 2009 09:58:08 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 8640A80E5; Mon, 23 Nov 2009 09:58:08 -0500 (EST) In-Reply-To: <20091123094431.GA31081@c3po> (Toby Cubitt's message of "Mon, 23 Nov 2009 09:44:31 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 2) Resent-Date: Mon, 23 Nov 2009 10:07:29 -0500 X-BeenThere: bug-gnu-emacs@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:32850 Archived-At: >> Hmm... using integers for "direction" isn't pretty. I see it mostly >> comes from its use in avl-tree--node-branch. I guess we'll have to live >> with it for now... > Exactly -- it stems from avl-tree--node-branch (which isn't my code). > The comment after its definition (also not mine) explains the reason > for it. I can't think of an efficient way to avoid this. I know. I wrote the comment ;-) >> > +(defun avl-tree--del-balance (node branch dir) >> > + ;; Rebalance a tree at the left (BRANCH=0) or right (BRANCH=1) child >> > + ;; of NODE after deleting from the left (DIR=0) or right (DIR=1) >> > + ;; sub-tree of that child [or is it vice-versa?]. Return t if the >> > + ;; height of the tree has shrunk. >> >> This comment should be a docstring instead. > I've followed the existing convention in avl-tree.el, which doesn't > provide docstrings for internal functions. If you want to change all the > comments to docstrings throughout, that's up to you, but I disagree. Maybe I'll do it at some point, but at least when changing code, the new code should use docstrings rather than comments. It used to be the case that docstrings were costly (in terms of memory use), but that was fixed around Emacs-19 so docstrings are not kept in memory any more. There is still a fair bit of code written before that time which still avoids using docstrings for that reason. While I don't think it's important to go back and fix all that old code, I think it's worthwhile to fix it when we touch such code. > I personally find the fact that internal library functions aren't > documented to be a useful form of documentation in itself: it tells me > that the function definitely shouldn't be used outside of the library. That's what the "--" is for in avl-tree--del-balance. It's also much better because you don't need to check "does this function have a docstring" to figure out whether it's internal or not. > Since Elisp doesn't have modules or private functions, that's the > closest one can get to an internal library function. Emacs abounds > with functions lacking docstrings, many of them apparently for > this reason. Some people follow this kind of convention, but the Emacs project doesn't. We only drop the docstring when we're lazy or when we don't know what to put in it. > If you still insist on making one or other or both of these into > docstrings, I'll make the necessary changes to the patches. Please do, thank you. >> Why exactly do you remove the \(fn TREE) thingy at the end? > It seemed to be a strange documentation convention followed only by > avl-tree.el, and inconsistently at that. It's redundant, and as far as I > know isn't used anywhere else in Emacs, because "C-h f " > already automatically shows the function's calling convention. It's a trick that can be used to *change* the calling convention displayed. So it's not used/needed often. But here, for example, it's used to change (avl-tree-compare-function CL-X) into (avl-tree-compare-function TREE), so the docstring (which refers to TREE) makes more sense. Sometimes it's used to hide some internal arguments. It's also used systematically in autoloaded definitions so as to provide the calling convention before the function is loaded. >> Overall, looks good. Please provide a ChangeLog entry for it as well. > Errrmmmm...do you mean a ChangeLog entry in the avl-tree.el file itself? > If so, I already did! :) No, an entry in the format used by the ChangeLog file. See http://www.gnu.org/prep/standards/standards.html#Change-Logs Stefan