lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Allows inheritence for slur engravers (issue 7437048)


From: dak
Subject: Allows inheritence for slur engravers (issue 7437048)
Date: Fri, 01 Mar 2013 14:01:21 +0000

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

That way it is clear that those values are for the life time of the
engraver.  It would make even more sense to declare them virtual, but
C++ only allows virtual functions, so one would have to make them
get_grob_name (), get_event_name_ () etc and that's probably too much of
a nuisance even though it would squeeze off a few bytes per instance.

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

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.

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.

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.

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.

https://codereview.appspot.com/7437048/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]