Thanks for the review! Cheers, MS
> Thanks for the rebase! > > > https://codereview.appspot.com/7437048/diff/1/lily/slur-engraver.cc > File lily/slur-engraver.cc (right): > > https://codereview.appspot.com/7437048/diff/1/lily/slur-engraver.cc#newcode51 > lily/slur-engraver.cc:51: event_name_ = "slur-event"; > As a matter of style, I'd declare all of those const and initialize them > in the initializer list, so > Slur_engraver::Slur_engraver () : grob_name_ ("Slur"), event_name_ ... Done. > > https://codereview.appspot.com/7437048/diff/1/lily/slur-engraver.cc#newcode64 > lily/slur-engraver.cc:64: context ()->set_property ("slurMelismaBusy", m > ? SCM_BOOL_T : SCM_BOOL_F); > Well, we have ly_bool2scm. Not a fabulous code saver, but as long as we > have it... Done. > > https://codereview.appspot.com/7437048/diff/1/lily/slur-engraver.cc#newcode70 > lily/slur-engraver.cc:70: internal_process_music (); > I'd rather make this function the default behavior of > Slur_proto_engraver::process_music > and make set_melisma a virtual function of Slur_proto_engraver that does > nothing by default and is overriden by Slur_engraver::set_melisma. Done. > > https://codereview.appspot.com/7437048/diff/1/lily/slur-proto-engraver.cc > File lily/slur-proto-engraver.cc (right): > > https://codereview.appspot.com/7437048/diff/1/lily/slur-proto-engraver.cc#newcode119 > lily/slur-proto-engraver.cc:119: slurs_[i]->warning (_ (("unterminated > "+warning_name_).c_str ())); > I'd not use warning_name_ but rather something like object_name_. After > all, there is no logical connection to warnings. Done. > https://codereview.appspot.com/7437048/diff/1/lily/slur-proto-engraver.cc#newcode138 > lily/slur-proto-engraver.cc:138: && make_double_slur_) > "doubleSlurs" is not generic but rather specific. I'd use something > like > if (double_property_name_ && to_boolean (get_property > (double_property_name_))) > instead, then we can, if desired, have a separate doublePhrasingSlur > property at some point of time. Done. Had to make the check slightly different: if ((double_property_name_ != "") && to_boolean (get_property (double_property_name_.c_str ()))) as GCC complains that there is no operator && for the check you're suggesting. > > https://codereview.appspot.com/7437048/diff/1/lily/slur-proto-engraver.cc#newcode183 > lily/slur-proto-engraver.cc:183: slur->programming_error ("slur without > a cause"); > warning_name_ (or, after my proposal, _object_name_) here. > Done. https://codereview.appspot.com/7437048/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
