Fixes boolean/SCM confusions, part 1.

Fixes boolean/SCM confusions, part 1.

[Cécile Hauchemaille, 2011-08-18 13:19:10]

Hi everyone,

I started working on this issue:
http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00646.html
Could you tell me if this is correct?

Regards,
Cécile Hauchemaille

[Bertrand Bordage, 2011-08-18 13:30:35]

LGTM :)

Bertrand

[Bertrand Bordage, 2011-08-18 13:31:23]

(Two minor comments however).

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc
File lily/auto-beam-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc#newco...
lily/auto-beam-engraver.cc:172: return !scm_is_false (scm_call_4 (get_property
("autoBeamCheck"),
Indent like this:

return !scm_is_false (scm_call_4 (
                        get_property ("autoBeamCheck"),
                        context ()->self_scm (),
[...]

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc
File lily/bar-check-iterator.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc#newco...
lily/bar-check-iterator.cc:56: SCM sync = tr->get_property
("barCheckSynchronize");
Maybe you should also merge this line and line 65?
But some will find it more convenient to see the properties defined here, so I
don't know what is better.

[Reinhold, 2011-08-18 13:32:17]

LGTM.
I haven't run the regtests, though, to make sure there are no differences.

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false (handle)
As we are calling cdr on the handle, it would probably be better to check for a
list using ly_is_list(handle)

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc
File lily/bar-check-iterator.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc#newco...
lily/bar-check-iterator.cc:52: if (to_boolean (tr->get_property
("ignoreBarChecks")))
scm_is_true

[Cécile Hauchemaille, 2011-08-18 14:03:02]

Patch updated.

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false (handle)
So, might scm_is_pair also work ?

[dak, 2011-08-18 14:08:03]

You'll find that I don't consider all advice given to you valid.  Which shows
that this is a difficult area to understand, and that we should likely, as a
byproduct of your work, do a writeup about "Guile and Lilypond" to make it
easier for others to just follow rules when writing stuff.

http://codereview.appspot.com/4875054/diff/1/lily/accidental-engraver.cc
File lily/accidental-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/accidental-engraver.cc#newc...
lily/accidental-engraver.cc:455: if (!ly_is_equal (info.grob ()->get_property
("style"),
Use scm_is_eq instead here: ly_is_equal is the C version of equal?, and you want
the simpler (and more efficient) eq? instead when comparing for equality with a
symbol.

eq? will actually be the same as == in effect, except that no type conversions
are triggered like in C's 0 == 0.0.

http://codereview.appspot.com/4875054/diff/1/lily/accidental-engraver.cc#newc...
lily/accidental-engraver.cc:498: if (!ly_is_equal (last_keysig_, sig))
I'd likely use scm_is_eq here as well: it is more efficient and will reliably
detect whether the keySignature has been set fresh.  You might argue that you
want the comparison to return true if it has been set to a value with the same
contents (equal? would do that), but that case is going to happen much less
frequently, and maybe update_local_key_signature also does things like
forgetting accidentals.

So better stay with the more equivalent scm_is_eq here.

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false (handle)
On 2011/08/18 13:32:17, Reinhold wrote:
> As we are calling cdr on the handle, it would probably be better to check for
a
> list using ly_is_list(handle)
ly_is_list is much less efficient (it traverses the list to make sure it is a
proper list), and scm_assoc can only return a pair or false.  Checking for
anything but #f after assoc is a distraction if what you are after is the
question of having found a match.

Indeed, the result of assoc is not generally a proper list but merely a pair, so
your advice is downright wrong.

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc
File lily/auto-beam-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc#newco...
lily/auto-beam-engraver.cc:172: return !scm_is_false (scm_call_4 (get_property
("autoBeamCheck"),
!scm_is_false can, somewhat counterintuitively, be replaced with scm_is_true
which also just compares with #f.

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc
File lily/bar-check-iterator.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/bar-check-iterator.cc#newco...
lily/bar-check-iterator.cc:52: if (to_boolean (tr->get_property
("ignoreBarChecks")))
On 2011/08/18 13:32:17, Reinhold wrote:
> scm_is_true

scm_is_true compares for any value different from #f while to_boolean admits
only #t as a true value.

Since unset values in Lilypond usually read out as '(), scm_is_true would be the
wrong thing to use here, resulting in a true value for an unset property.

[Reinhold, 2011-08-18 14:18:39]

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false (handle)
On 2011/08/18 14:03:06, Cécile Hauchemaille wrote:
> So, might scm_is_pair also work ?

Yes, of course, that's the correct check...

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc
File lily/auto-beam-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/auto-beam-engraver.cc#newco...
lily/auto-beam-engraver.cc:172: return !scm_is_false (scm_call_4 (get_property
("autoBeamCheck"),
On 2011/08/18 13:31:26, Bertrand Bordage wrote:
> Indent like this:
> 
> return !scm_is_false (scm_call_4 (
>                         get_property ("autoBeamCheck"),
>                         context ()->self_scm (),
> [...]

Or more correct: align the context ()->self_scm () with the get_property:

return !scm_is_false (scm_call_4 (get_property ("autoBeamCheck"),
                                  context ()->self_scm (),
                                  ...

[Neil Puttock, 2011-08-18 14:24:43]

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc
File lily/accidental-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc#n...
lily/accidental-engraver.cc:319: if (scm_to_int (left_objects_[i]->get_property
("side-axis")) == X_AXIS)
this is potentially unsafe, but only since the type predicate for side-axis is
number? rather than integer? (a type predicate exclusive to axis is probably not
worth the effort)

[c_sorensen, 2011-08-18 14:25:13]

On 8/18/11 8:18 AM, "reinhold.kainhofer@gmail.com"
<reinhold.kainhofer@gmail.com> wrote:

>> Indent like this:
> 
>> return !scm_is_false (scm_call_4 (
>>                          get_property ("autoBeamCheck"),
>>                          context ()->self_scm (),
>> [...]
> 
> Or more correct: align the context ()->self_scm () with the
> get_property:
> 
> return !scm_is_false (scm_call_4 (get_property ("autoBeamCheck"),
>                                    context ()->self_scm (),
>                                    ...
Or, just use fixcc.py on the file.  By definition, fixcc.py creates the
correct indentation.

Thanks,

Carl

[dak, 2011-08-18 15:32:20]

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc
File lily/accidental-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc#n...
lily/accidental-engraver.cc:319: if (scm_to_int (left_objects_[i]->get_property
("side-axis")) == X_AXIS)
On 2011/08/18 14:24:44, Neil Puttock wrote:
> this is potentially unsafe, but only since the type predicate for side-axis is
> number?

I can't see that there is a safety problem: scm_to_int will return an integer if
an integer can be returned, or will throw an error.  We should definitely change
the type predicate to integer? to avoid this kind of error being
thrown at a less expected time, but that's about it.

The time to worry about nonsensical settings is when writing, not reading
variables.  After all, the worst that can happen is a controlled exit here.

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc#n...
lily/accidental-engraver.cc:455: if (!ly_is_equal (info.grob ()->get_property
("style"),
See comment for the last patch iteration.  Use scm_is_eq here for efficiency.

http://codereview.appspot.com/4875054/diff/8002/lily/accidental-engraver.cc#n...
lily/accidental-engraver.cc:498: if (!ly_is_equal (last_keysig_, sig))
Again, use scm_is_eq here.  Explanation was in last comment batch already.

[dak, 2011-08-18 15:46:43]

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):

http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false (handle)
On 2011/08/18 14:18:45, Reinhold wrote:
> On 2011/08/18 14:03:06, Cécile Hauchemaille wrote:
> > So, might scm_is_pair also work ?
> 
> Yes, of course, that's the correct check...

It is a check that will work correctly.  However, it is not idiomatic.  The
description for assoc and its friends will state that assoc will return #f when
no association could be found.  It does not state "will return something that is
not a pair", even though #f is the only possible non-pair value.

So in order to match the check best to the description of the function, I'd
really check against false.

scm_is_pair will definitely work just as reliably for the computer, but I think
for humans, the check for false requires less thinking.

http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc
File lily/bar-check-iterator.cc (right):

http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#ne...
lily/bar-check-iterator.cc:52: if (scm_is_true (tr->get_property
("ignoreBarChecks")))
As I already explained: you can't replace to_boolean with scm_is_true, since
to_boolean( '() ) -> 0, whereas scm_is_true ( '() ) -> 1 (I apologize for the
mixture of X and Scheme syntax), and '() is _definitely_ one value that needs to
be interpreted as false in the context of boolean properties.

http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#ne...
lily/bar-check-iterator.cc:64: if (scm_is_true (tr->get_property
("barCheckSynchronize")))
Again, scm_is_true is wrong here.

[Carl, 2011-08-18 18:01:36]

On 2011/08/18 15:46:43, dak wrote:

>
http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#ne...
> lily/bar-check-iterator.cc:52: if (scm_is_true (tr->get_property
> ("ignoreBarChecks")))
> As I already explained: you can't replace to_boolean with scm_is_true, since
> to_boolean( '() ) -> 0, whereas scm_is_true ( '() ) -> 1 (I apologize for the
> mixture of X and Scheme syntax), and '() is _definitely_ one value that needs
to
> be interpreted as false in the context of boolean properties.

It depends.  If you want to have unset properties default to #t, then
scm_is_true is correct.

If you want to have them default to #f, then to_boolean is clearly the correct
way to go.

If you want to have a different default, depending on the property, then we
should have a robust_scm2bool that should be used in these cases.

[Cécile Hauchemaille, 2011-08-18 20:46:37]

Thanks for your prompt and detailed reviews!
I applied the changes, especially dak's. I changed the type of side-axis
property to an integer.

Regards,
Cécile Hauchemaille

[dak, 2011-08-18 21:04:21]

On 2011/08/18 18:01:36, Carl wrote:
> On 2011/08/18 15:46:43, dak wrote:
> 
> >
>
http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#ne...
> > lily/bar-check-iterator.cc:52: if (scm_is_true (tr->get_property
> > ("ignoreBarChecks")))
> > As I already explained: you can't replace to_boolean with scm_is_true, since
> > to_boolean( '() ) -> 0, whereas scm_is_true ( '() ) -> 1 (I apologize for
the
> > mixture of X and Scheme syntax), and '() is _definitely_ one value that
needs
> to
> > be interpreted as false in the context of boolean properties.
> 
> It depends.  If you want to have unset properties default to #t, then
> scm_is_true is correct.
> 
> If you want to have them default to #f, then to_boolean is clearly the correct
> way to go.
> 
> If you want to have a different default, depending on the property, then we
> should have a robust_scm2bool that should be used in these cases.

Well, if we have properties that should default to #t (do we really have any of
those?) then we should likely use something like to_boolean_or_t and
to_boolean_or_f instead of scm_is_true and to_boolean.  Using the latter is far
too obscure and does not make the programmer's intention obvious.

[Carl, 2011-08-18 21:43:38]

On 2011/08/18 21:04:21, dak wrote:
> On 2011/08/18 18:01:36, Carl wrote:
> > On 2011/08/18 15:46:43, dak wrote:

> 
> Well, if we have properties that should default to #t (do we really have any
of
> those?) then we should likely use something like to_boolean_or_t and
> to_boolean_or_f instead of scm_is_true and to_boolean.  Using the latter is
far
> too obscure and does not make the programmer's intention obvious.

My quick review of the user backend properties in Internals showed that all of
the boolean properties I could find should default to #f.  So using to_boolean
is probably the right way to go.

I think we ought to have a comment somewhere that describes why we don't use
scm_is_true.  But I can't figure out where to put it -- I guess it should be in
the documentation that we hope will arise as a result of all this effort.

[Graham Percival, 2011-08-18 23:30:57]

On Thu, Aug 18, 2011 at 09:43:38PM +0000, Carl.D.Sorensen@gmail.com wrote:
> I think we ought to have a comment somewhere that describes why we don't
> use scm_is_true.  But I can't figure out where to put it -- I guess it
> should be in the documentation that we hope will arise as a result of
> all this effort.

Yes, but let's not "hope" for this.

Bertrand: you're her mentor, so this is your job.  Add a section
to the CG chapter on "programming".  Make a new @section right
before "LilyPond miscellany".  You'll need to add it to the @menu
as well.  Check that the docs still compile by running "make", and
as long as that doesn't die, then push that change directly.  Just
write "stub" in the text for that @section.

Bertrand, second stage: write anything you want in there.  You
could copy portions of David's emails verbatim, or just copy bits
and pieces of emails and put them into an itemized list, or
whatever.  Or you could ask Cecile to write something.  Again,
push directly with no review as long as the documentation still
completes a plain old "make".

Everybody else: feel free to jump in and correct that part of the
CG if you notice anything that's incorrect or unclear.  No
reitveld reviews; just change something and push it if you're
certain, or send a question (in a new thread) to lilypond-devel if
you're uncertain.

Cheers,
- Graham