lily-guile updates and CG: "Scheme->C interface" section.

lily-guile updates and CG: "Scheme->C interface" section.

[Bertrand Bordage, 2011-08-19 17:05:37]

Hi,

Graham asked me to document an small issue.
(http://codereview.appspot.com/4875054/)
And this became something bigger.

He also told me to push it directly, but I made a few changes in the interface
as I read the code and wrote the doc.
So the review concerns only the C files of this patch, but feel free to tell me
if I was totally wrong when writing it.

Thanks,
Bertrand

[Graham Percival (old account), 2011-08-19 17:10:03]

Doc part LGTM.  I can't speak about the scm / C++ stuff.

[Carl, 2011-08-19 18:04:38]

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/progra...
File Documentation/contributor/programming-work.itexi (right):

http://codereview.appspot.com/4917044/diff/1/Documentation/contributor/progra...
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/progra...
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/progra...
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/progra...
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/progra...
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#Str...>
)

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

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

[Neil Puttock, 2011-08-19 20:20:12]

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))
On 2011/08/19 18:04:38, Carl wrote:

> I think the old code here is correct, and the doc-string is wrong.

Both are correct; the only difference is when Guile raises an error.  In
Bertrand's version, it's at the top of the code; in current master, it's in the
block below when scm_to_int () is called.

http://codereview.appspot.com/4917044/diff/1/lily/include/lily-guile.hh
File lily/include/lily-guile.hh (right):

http://codereview.appspot.com/4917044/diff/1/lily/include/lily-guile.hh#newco...
lily/include/lily-guile.hh:96: //                       or scm_is_true
(scm_exact_p (x)) ?
Have you looked at the definition in the Guile repository (libguile/numbers.h)?

It's a low-level function which returns true if the SCM object isn't an
immediate
(http://www.gnu.org/software/guile/manual/guile.html#Immediate-objects) and is
tagged as a fraction (I think this only happens if scm_divide () creates a
fraction)

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#newcode229
lily/lily-guile.cc:229: return is_number_pair (p) ?
ly_scm2interval is deliberately sloppy; it's assumed the caller has made sure
the object is a number pair.  If we're unsure, we use robust_scm2interval, which
does the check then calls this function to perform the conversion.  Adding this
extra safety check will cause any robust_scm2interval call on a valid
number-pair to get two checks.

http://codereview.appspot.com/4917044/diff/1/lily/lily-guile.cc#newcode290
lily/lily-guile.cc:290: return is_number_pair (s) ?
see comment for ly_scm2interval

[Neil Puttock, 2011-08-19 20:22:08]

On 2011/08/19 20:20:12, Neil Puttock wrote:

> http://codereview.appspot.com/4917044/diff/1/lily/include/lily-guile.hh
> File lily/include/lily-guile.hh (right):
> 
>
http://codereview.appspot.com/4917044/diff/1/lily/include/lily-guile.hh#newco...
> lily/include/lily-guile.hh:96: //                       or scm_is_true
> (scm_exact_p (x)) ?
> Have you looked at the definition in the Guile repository
(libguile/numbers.h)?
> 
> It's a low-level function

I mean macro, of course.

[Bertrand Bordage, 2011-08-19 20:22:27]

On 2011/08/19 18:04:38, Carl wrote:
> THanks for doing this!

And thanks for this excellent review!
 
> 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.

You're right.  Specifying the meaning of the prefixes "ly_", "robust_", ...
should be enough, but requires to unify these functions (name and default
value).  As you can see, I made a few changes in this direction.  If you think
this is a good idea, I can unify the whole interface.  Of course, there's a lot
of downstream name changes to do.  And maybe make a small changelog somewhere
for the developpers.

> I think the code changes should be separated from the doc changes.

This is two commits I uploaded at once.  Since I wrote the small references
according to the changes made to the C files, I thought this would be easier to
understand.

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

Ok, so there's no logical error.  Forgive me.

> http://codereview.appspot.com/4917044/diff/1/lily/general-scheme.cc#newcode110
> 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.

So I guess the scm_to_int has to be changed for scm_to_double in these files...
http://codereview.appspot.com/4917044/diff/1/lily/lily-guile.cc#newcode217
http://codereview.appspot.com/4917044/diff/1/lily/general-scheme.cc#newcode112

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

This could be very nice!


I totally agree with anything else.
I'll make a new patch set before Tuesday.

Thanks,
Bertrand

[Carl, 2011-08-19 21:08:10]

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))
On 2011/08/19 20:20:12, Neil Puttock wrote:
> On 2011/08/19 18:04:38, Carl wrote:
> 
> > I think the old code here is correct, and the doc-string is wrong.
> 
> Both are correct; the only difference is when Guile raises an error.  In
> Bertrand's version, it's at the top of the code; in current master, it's in
the
> block below when scm_to_int () is called.

OK, I see now.  I withdraw my objection to these changes.

As an aside, I think that we should change the definition of the property
align-dir.  It should no longer be called a direction, since it's not limited to
the values -1, 0, and 1.

But this is a discussion for another thread.

Thanks!

[Graham Percival, 2011-08-19 22:35:35]

On Fri, Aug 19, 2011 at 06:04:38PM +0000, Carl.D.Sorensen@gmail.com wrote:
> 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.

On the long term, I agree with Carl.  In the short term, I think
the docs should be pushed "as-is".  It's easy to make editorial
fixes later; I want us (meaning Bertrand and the programmers
commenting on this stuff) to get into the habit of archiving
information first, and playing with formatting later.

Cheers,
- Graham

[Neil Puttock, 2011-08-19 23:03:37]

On 2011/08/19 21:08:10, Carl wrote:

> As an aside, I think that we should change the definition of the property
> align-dir.  It should no longer be called a direction, since it's not limited
to
> the values -1, 0, and 1.

It's unused.  I think it's superseded by stacking-dir.

Cheers,
Neil

[c_sorensen, 2011-08-19 23:07:14]

On 8/19/11 4:35 PM, "Graham Percival" <graham@percival-music.ca> wrote:

> On Fri, Aug 19, 2011 at 06:04:38PM +0000, Carl.D.Sorensen@gmail.com wrote:
>> 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.
> 
> On the long term, I agree with Carl.  In the short term, I think
> the docs should be pushed "as-is".  It's easy to make editorial
> fixes later; I want us (meaning Bertrand and the programmers
> commenting on this stuff) to get into the habit of archiving
> information first, and playing with formatting later.

I totally agree.

Carl

[Carl, 2011-08-20 06:24:45]

On 2011/08/19 23:03:37, Neil Puttock wrote:
> On 2011/08/19 21:08:10, Carl wrote:
> 
> > As an aside, I think that we should change the definition of the property
> > align-dir.  It should no longer be called a direction, since it's not
limited
> to
> > the values -1, 0, and 1.
> 
> It's unused.  I think it's superseded by stacking-dir.
> 

It's used in fret-diagram markups.  See Notation Reference A.9.5.

Thanks,

Carl

[Bertrand Bordage, 2011-09-22 11:41:39]

Update done.
I think the C part is ok.
There's maybe a few things to change in the doc.

Shall I wait for a countdown or push directly?

Bertrand

[pkx166h, 2011-09-22 19:24:19]

Couldn't find a tracker issue for this, however it passes make and reg tests.

[Graham Percival (old account), 2011-09-22 20:53:39]

Since it has C changes as well, I'd prefer it to go through a countdown.

[Bertrand Bordage, 2011-10-18 13:10:43]

I pushed the doc as b4a2cb2cf00347c477ed595f1435cc212e70ce33.

Could the remaining C part of the patch be 'countdowned'?

[Bertrand Bordage, 2011-10-23 20:58:15]

"lily-guile updates" pushed as 6ee8c04678442855cb794d4598c056c15c42673b.

[dak, 2011-10-23 21:23:11]

As usual, too late in the game.  Better late than never.

http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/pr...
File Documentation/contributor/programming-work.itexi (right):

http://codereview.appspot.com/4917044/diff/16006/Documentation/contributor/pr...
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/pr...
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/pr...
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/pr...
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/pr...
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/pr...
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.

[Bertrand Bordage, 2011-10-25 17:00:37]

Hi David,

Could you make your own patch for the doc changes?
And, as you mentionned, the unused function should be removed.  Do you want me
to commit this change?

Bertrand

[dak, 2011-10-25 20:51:48]

bordage.bertrand@gmail.com writes:

> Hi David,
>
> Could you make your own patch for the doc changes?

Pushed.  I have enough pending reviews, commits and patches that I don't
have the nerve to do yet another hoop-jumping exercise in parallel.

> And, as you mentionned, the unused function should be removed.  Do you
> want me to commit this change?

That would be the smart thing to do.  Broken unused code does not help
anybody.

-- 
David Kastrup