THanks for doing this! I have some comments about the docs. I think they're too tutorial, and I think the exhaustive lists are unwieldy and should be eliminated. THe source should be the reference.
I think the code changes should be separated from the doc changes. I disagree with your changes on directions; they don't need to be integers. Thanks, Carl http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi File Documentation/contributor/programming-work.itexi (right): http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1811 Documentation/contributor/programming-work.itexi:1811: (see @ref{LilyPond programming languages}). I'd prefer to see a URL to the appropriate Guile page here. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1830 Documentation/contributor/programming-work.itexi:1830: or warning when compiling but must be avoided: I think that you should reference the paragraph that David pointed out in his email: <http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00646.html> The code in your example *will work*, but it is not following the API guidelines. So we should be clear about why we're doing this. It's for standards compliance, not because of lack of functionality. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1833 Documentation/contributor/programming-work.itexi:1833: scm_string_p ([any_SCM_you_want]) == SCM_BOOL_T I think it would be better to say: if (scm_string_p (scm_value) == SCM_BOOL_T) i.e. put in a real variable name, instead of a place holder) http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1840 Documentation/contributor/programming-work.itexi:1840: quicker in terms of execution time. Are you sure of the execution time argument? I'm not. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1845 Documentation/contributor/programming-work.itexi:1845: a misnomer in the reference: it says @code{scm_is_string} returns @code{#t} Are you sure this is correct? The current page of the API reference says: — Scheme Procedure: string? obj — C Function: scm_string_p (obj) Return #t if obj is a string, else #f. — C Function: int scm_is_string (SCM obj) Returns 1 if obj is a string, 0 otherwise. (see <http://www.gnu.org/software/guile/manual/html_node/String-Predicates.html#String-Predicates> ) Also, the Guile API says : The type of the return value of a C function that corresponds to a Scheme function is always SCM. In the descriptions below, types are therefore often omitted but for the return value and for the arguments. (see <http://www.gnu.org/software/guile/manual/html_node/API-Overview.html#API-Overview> ) So scm_is_string returns a C int, with a value of 0 or 1. scm_string_p returns a C SCM, with a value of #t or #f (but we can't access those values directly from C). I think the key point that we should get across (which I didn't understand until I started this review) is that we should use generally use C functions and macros that have no corresponding Scheme procedures. The C functions with corresponding Scheme procedures return SCM values, and we can't use them for anything but assignment in C. Or perhaps it's clearer to say that for anything but assignment in C, we should use only C Functions and Macros with non-SCM return values. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1876 Documentation/contributor/programming-work.itexi:1876: Here is a list of these functions: Rather than explaining these functions, I think we should only explain cases where the usage is non-obvious (i.e. using to_boolean on property returns, since unset properties often aren't boolean values. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1878 Documentation/contributor/programming-work.itexi:1878: TODO: complete this list. See my comment below. I don't think we should have exhaustive lists, unless they are automatically generated. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1882 Documentation/contributor/programming-work.itexi:1882: Convert a Scheme boolean @var{b} to a C boolean, else return false. If b is a Scheme boolean value, return the corresponding C boolean value, else return false. Or Return @code{true} if @var{b} is @code{SCM_BOOL_T}, otherwise return @code{false} http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1918 Documentation/contributor/programming-work.itexi:1918: TODO: complete this list. I'm not in favor of having a complete list here. Unless we can identify a means of automatically creating the list from the source code, this will invariably get out of date. Better not to have it than to have it be out of date. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1927 Documentation/contributor/programming-work.itexi:1927: @subsubheading Interval ly_scm2interval (SCM p) Rather, I should say it converts a Scheme interval (which happens to be a pair of numbers) to a C Interval. http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/programming-work.itexi#newcode1934 Documentation/contributor/programming-work.itexi:1934: Convert a pair of floating point numbers @var{p} to an offset, Similarly, it converts a Scheme offset to a C offset. http://codereview.appspot.com/4917044/diff/1/lily/general-scheme.cc File lily/general-scheme.cc (right): http://codereview.appspot.com/4917044/diff/1/lily/general-scheme.cc#newcode110 lily/general-scheme.cc:110: if (scm_is_integer (s)) Is this correct? Should directions be limited to integers? For example, the backend property align-dir is a direction. It can be set anywhere between -1 and 1, so that I can center two grobs, or left align them, or right align them, or align them with a bias to the left or to the right. 0.5 and -0.5 are valid values for align-dir. I think the old code here is correct, and the doc-string is wrong. The code expresses that any number between -1 and 1 is a valid direction, not just the integers -1, 0 and 1. I think this change should not be pushed. http://codereview.appspot.com/4917044/diff/1/lily/lily-guile.cc File lily/lily-guile.cc (right): http://codereview.appspot.com/4917044/diff/1/lily/lily-guile.cc#newcode181 lily/lily-guile.cc:181: if (scm_is_integer (s)) I think integer is correct here. We don't have axes that are halfway between Xand Y. http://codereview.appspot.com/4917044/diff/1/lily/lily-guile.cc#newcode215 lily/lily-guile.cc:215: if (scm_is_integer (s)) I think number was correct. Directions are not limited to -=1, 0, 1. THey can be any number between -1 and 1. See earlier comment. http://codereview.appspot.com/4917044/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel