From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Toby Cubitt <tsc25@cantab.net>
Cc: bug-gnu-emacs@gnu.org
Subject: bug#5015: avl-tree.el enhancements
Date: Sun, 22 Nov 2009 22:44:22 -0500 [thread overview]
Message-ID: <jwvk4xi2b0k.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <20091123000123.GA7647@c3po> (Toby Cubitt's message of "Mon, 23 Nov 2009 00:01:23 +0000")
> I've initiated the copyright paperwork process, so hopefully you should
> have everything you need soon.
Great. In the mean time, here's some comment about the first patch.
> -;; along with GNU Emacs. If not, see <http://www.gnu.org/licenses/>.
> +;; along with GNU Emacs. If not, see <http://www.gnu.org/licenses/>.
This change is wrong. Our convention is to use two spaces after a full
stop. See sentence-end-double-space. Please try to follow the same
convention in the text you contribute.
> - `(avl-tree--node-left (avl-tree--dummyroot tree)))
> + `(avl-tree--node-left (avl-tree--dummyroot ,tree)))
Good catch, thanks.
> +;; ----------------------------------------------------------------
> +;; Convenience macros
> +
> +(defmacro avl-tree--switch-dir (dir)
> + ;; Return opposite direction to DIR (0 = left, 1 = right).
> + `(- 1 ,dir))
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...
> +(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.
> +(defun avl-tree--mapc (map-function root dir)
> ;; Apply MAP-FUNCTION to all nodes in the tree starting with ROOT.
> - ;; The function is applied in-order.
> + ;; The function is applied in-order, either ascending (DIR=0) or
> + ;; descending (DIR=1).
> ;;
> ;; Note: MAP-FUNCTION is applied to the node and not to the data itself.
> ;; INTERNAL USE ONLY.
Same here: this should be a docstring, rather than a comment.
> -(defalias 'avl-tree-compare-function 'avl-tree--cmpfun
> - "Return the comparison function for the avl tree TREE.
> +;; define public alias for constructors so that we can set docstring
> +(defalias 'avl-tree-create 'avl-tree--create
> + "Create an empty avl tree.
> +COMPARE-FUNCTION is a function which takes two arguments, A and B,
> +and returns non-nil if A is less than B, and nil otherwise.")
> +
> -\(fn TREE)")
> +(defalias 'avl-tree-compare-function 'avl-tree--cmpfun
> + "Return the comparison function for the avl tree TREE.")
Why exactly do you remove the \(fn TREE) thingy at the end?
> - (let ((node (avl-tree--root tree))
> - (compare-function (avl-tree--cmpfun tree))
> - found)
> - (while (and node
> - (not found))
> - (cond
> - ((funcall compare-function data (avl-tree--node-data node))
> - (setq node (avl-tree--node-left node)))
> - ((funcall compare-function (avl-tree--node-data node) data)
> - (setq node (avl-tree--node-right node)))
> - (t
> - (setq found t))))
> - (if node
> - (avl-tree--node-data node)
> + (let ((node (avl-tree--root tree)))
> + (catch 'found
> + (while node
> + (cond
> + ((funcall (avl-tree--cmpfun tree)
> + data (avl-tree--node-data node))
> + (setq node (avl-tree--node-left node)))
> + ((funcall (avl-tree--cmpfun tree)
> + (avl-tree--node-data node) data)
> + (setq node (avl-tree--node-right node)))
> + (t (throw 'found (avl-tree--node-data node)))))
> nil)))
Why do you move the (avl-tree--cmpfun tree) back into the loop?
Have you found it to be paradoxically more efficient?
Overall, looks good. Please provide a ChangeLog entry for it as well.
Stefan
next prev parent reply other threads:[~2009-11-23 3:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-11-23 0:01 bug#5012: avl-tree.el enhancements Toby Cubitt
2009-11-23 3:44 ` Stefan Monnier [this message]
2009-11-23 9:44 ` bug#5016: " Toby Cubitt
2009-11-23 14:58 ` bug#5021: " Stefan Monnier
2009-11-23 16:59 ` bug#5024: " Toby Cubitt
2009-11-23 17:16 ` bug#5012: " Glenn Morris
2012-02-23 19:51 ` Glenn Morris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwvk4xi2b0k.fsf-monnier+emacs@gnu.org \
--to=monnier@iro.umontreal.ca \
--cc=5015@emacsbugs.donarmstrong.com \
--cc=bug-gnu-emacs@gnu.org \
--cc=tsc25@cantab.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).