On Mon, Nov 23, 2009 at 09:58:08AM -0500, Stefan Monnier wrote:
> >> 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 ;-)

:-)

> > 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.

I see. I wasn't aware that the "--" convention was widely followed for
internal library functions (at least, widely enough to be understood). In
that case, you're of course right.

Is this convention documented anywhere? Should it be? I can't find it
mentioned anywhere in Appendix D of Emacs Lisp Manual...

> > 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.

I've fixed this in the new versions of the patches (attached). Since my
changes touch a lot of functions in avl-tree.el, I've gone through and
changed as many comments to docstrings as I can (I think I've caught all
of them, in fact).

> >> 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 <function>"
> > 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.

Always good to learn something new about Emacs :) I've reverted this
change in the attached patches. Now I'll have to go back through my other
Elisp code to see if this trick would be useful!

> >> 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

Ah, OK. I've attached a ChangeLog file with the entries for these
changes. I've tried to follow the guidelines in the link, but you'd
better check and let me know if they're OK.

Toby