Re: Fixes boolean/SCM confusions, part 1. (issue 4875054)

[dak]

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#newcode455
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#newcode498
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#newcode172
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#newcode52
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.

http://codereview.appspot.com/4875054/