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

[Carl . D . Sorensen]

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/