Re: lily-guile updates and CG: "Scheme->C interface" section. (issue 4917044)

[dak]

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/