Allows inheritence for slur engravers

Allows inheritence for slur engravers

[dak, 2013-03-01 14:01:21]

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#new...
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#new...
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#new...
lily/slur-proto-engraver.cc:183: slur->programming_error ("slur without a
cause");
warning_name_ (or, after my proposal, _object_name_) here.

[MikeSol, 2013-03-01 14:44:55]

From David's suggestions

[mike7, 2013-03-01 14:45:27]

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#new...
> 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#new...
> 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#new...
> 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/

[MikeSol, 2013-03-01 15:51:56]

Fixes slur double

[dak, 2013-03-01 16:30:51]

On 2013/03/01 14:45:27, mike7 wrote:

> > "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 ())))

Oh, I'd have made double_property_name_ a const char * const instead
of a string.  That can be 0, and you don't need to c_str on it.

> as GCC complains that there is no operator && for the check you're suggesting.

[mike7, 2013-03-01 17:00:28]

On 1 mars 2013, at 17:30, dak@gnu.org wrote:

> On 2013/03/01 14:45:27, mike7 wrote:
> 
>> > "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 ())))
> 
> Oh, I'd have made double_property_name_ a const char * const instead
> of a string.  That can be 0, and you don't need to c_str on it.
> 
>> as GCC complains that there is no operator && for the check you're
> suggesting.

I've always avoided using const char * because I never understood how it works
with memory management.

Given that we are creating a pointer (an array of chars), wouldn't we have to
delete it in the destructor?  Furthermore, how does GCC know to allocate the
correct amount of memory for the string?  Wouldn't that change depending on the
length of the string?

That's probably the sort of thing you learn in a Freshman CS class...sorry for
the basic question...

Cheers,
MS

[dak, 2013-03-01 17:51:36]

On 2013/03/01 17:00:28, mike7 wrote:
> On 1 mars 2013, at 17:30, mailto:dak@gnu.org wrote:
> 
> > On 2013/03/01 14:45:27, mike7 wrote:
> > 
> >> > "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 ())))
> > 
> > Oh, I'd have made double_property_name_ a const char * const instead
> > of a string.  That can be 0, and you don't need to c_str on it.
> > 
> >> as GCC complains that there is no operator && for the check you're
> > suggesting.
> 
> I've always avoided using const char * because I never understood how it works
> with memory management.
> 
> Given that we are creating a pointer (an array of chars),

Uh, a pointer.  _Not_ an array of chars.  The array is "xxx".  We don't create
that array, it is statically allocated by the compiler and remains the same over
the life time of the program.  We just set the pointer to point there.

> wouldn't we have to
> delete it in the destructor?  Furthermore, how does GCC know to allocate the
> correct amount of memory for the string?  Wouldn't that change depending on
the
> length of the string?
> 
> That's probably the sort of thing you learn in a Freshman CS class...

That's C knowledge glossed over in C++.  If you write "whatever", this allocates
a static array in constant memory with the respective characters
(null-terminated) in them and returns a pointer to that static array.  C is free
to assume that you will not overwrite that memory, so "xxx" and "xxx", written
in two different source locations, may or may not use the same memory location.

const char * does not work at all with memory management: it just takes that
pointer and uses it.  If it is from "xxx", you better never call free or delete
on that.

[MikeSol, 2013-03-01 18:36:56]

Uses const char* instead of string in several places

[dak, 2013-03-08 13:03:12]

LGTM