As usual, too late in the game. Better late than never.
http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi File Documentation/contributor/programming-work.itexi (right): http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1871 Documentation/contributor/programming-work.itexi:1871: (scm_list_p (scm_value) && scm_value != SCM_EOL) scm_list_p is _always_ true according to C since it is either SCM_BOOL_T or SCM_BOOL_F, both of which are non-zero. http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1880 Documentation/contributor/programming-work.itexi:1880: since a list of at least one member is considered as a pair. But not every pair is a proper list. scm_list_[ returns #t only for proper lists, not circular lists and not dotted lists. Still, this check is usually what one wants since it is cheap (list? goes through the whole list with a hare/tortoise algorithm). It just won't catch the degenerate cases. http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1884 Documentation/contributor/programming-work.itexi:1884: interface. As a rule of thumb, you get an scm_is_[something] for cheap predicates, those that are likely to be inlined. So you have scm_is_pair, but not scm_is_list, and scm_is_eq but not scm_is_eqv or scm_is_equal. http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1892 Documentation/contributor/programming-work.itexi:1892: This should be used instead of @code{scm_is_true} and @code{scm_is_false} Both scm_is_true and scm_is_false compare only for equality with #f, like if does. http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1893 Documentation/contributor/programming-work.itexi:1893: for properties since empty lists are sometimes used to unset them. since reading any unset property returns an empty list, and Lilypond has the convention to interpret unset boolean properties as false. http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/programming-work.itexi#newcode1904 Documentation/contributor/programming-work.itexi:1904: @node Conversion The whole node is duplication. http://codereview.appspot.com/4917044/diff/16006/lily/lily-guile.cc File lily/lily-guile.cc (left): http://codereview.appspot.com/4917044/diff/16006/lily/lily-guile.cc#oldcode339 lily/lily-guile.cc:339: return true; This returns true if every pair in list a is also in list b. But list b can contain more key-value pairs than a. Since an alist can contain duplicate entries (the former shadowing the latter), counting members is not sufficient to deduce equivalence. Also if a contains shadowed (different) entries, the test will never turn out #t. For example, this returns false when comparing '((#t . #t) (#t . #f)) with itself. If this function is indeed not used, delete this totally broken thing that can't be easily fixed. http://codereview.appspot.com/4917044/diff/16006/lily/lily-guile.cc File lily/lily-guile.cc (right): http://codereview.appspot.com/4917044/diff/16006/lily/lily-guile.cc#newcode325 lily/lily-guile.cc:325: // This one is used nowhere. If it is used nowhere, it should be removed. It is catastrophically wrong. http://codereview.appspot.com/4917044/diff/16006/lily/lily-guile.cc#newcode329 lily/lily-guile.cc:329: if (!scm_is_pair (a) || !scm_is_pair (b)) This returns false even if both alists are empty. http://codereview.appspot.com/4917044/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel