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/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
