accidental changes: review

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

accidental changes: review

Han-Wen Nienhuys-3
>-      SCM key = scm_cons (scm_from_int (o), scm_from_int (n));
>+      Duration *dur = unsmob_duration (note->get_property ("duration"));
>+
>+      SCM smp = get_property ("measurePosition");
>+      Moment mp = robust_scm2moment (smp, Moment (0));
>+      /*
>+ TODO: Check this. Is this correct? -rz :
>+       */
>+      Moment end_mp = mp.grace_part_ < Rational(0)
>+ ? Moment(mp.main_part_, mp.grace_part_+dur->get_length())
>+ : Moment(mp.main_part_+dur->get_length(), mp.grace_part_);

grace_part_ can never be larger than zero, so you could simplify to

        : Moment(mp.main_part_+dur->get_length(), 0);

other than that, it looks ok.

>--- /dev/null
>+  int octave_;
>+  int notename_;
>+  Rational alteration_;

Why don't you use a Pitch object for this combination? You would get
Scale* for free.

>+  bool has_position_;

Can you document what this does?

>--- a/lily/key-engraver.cc
>+++ b/lily/key-engraver.cc
>@@ -15,6 +15,7 @@
> #include "protected-scm.hh"
> #include "staff-symbol-referencer.hh"
> #include "stream-event.hh"
>+#include "key-entry.hh"
>
> #include "translator.icc"
>
>@@ -72,36 +73,56 @@ Key_engraver::create_key (bool is_default)
>   || key == SCM_EOL)

could you add a comment on what is happening here?

>    {
>-      SCM new_alter_pair = scm_assoc (scm_caar (s), key);
>-      Rational old_alter = robust_scm2rational (scm_cdar (s), 0);
>-      if (new_alter_pair == SCM_BOOL_F
>+      Key_entry *old_entry = Key_entry::unsmob (scm_car (s));
>+      Key_entry *new_entry = NULL;
>+      for (SCM t = key; scm_is_pair (t); t = scm_cdr (t))
>+ {
>+  Key_entry *entry = Key_entry::unsmob (scm_car (t));
>+  if ( old_entry->get_notename () == entry ->
>get_notename ())

no space after (

>+    {
>+      new_entry = entry;
>+      break;
>+    }
>+ }
>+      Rational old_alter = old_entry->get_alteration ();
>+      Rational new_alter = new_entry->get_alteration ();
>+      SCM old_alterpair = old_entry -> to_name_alter_pair ();

no space around ->

>-      item_->set_property ("alteration-alist", key);
>+      SCM key_scm = scm_list_copy (key);
>+      for (SCM s = key_scm; scm_is_pair (s); s = scm_cdr (s))
>+ {
>+  Key_entry *entry = Key_entry::unsmob (scm_car (s));
>+  *SCM_CARLOC (s) = entry -> to_name_alter_pair ();

del space around ->


>+
>+
>+IMPLEMENT_TYPE_P (Key_entry, "ly:key-signature-entry?");
>+
>+SCM
>+Key_entry::mark_smob (SCM x)
>+{
>+  return SCM_EOL; // FIXME: I don't understand what this function is
>supposed to do. This is just blindly coppied from moment.cc

call scm_gc_mark() on all SCM values that are inside the smob. You can
return one of them iso. scm_gc_mark()ing them.

>+LY_DEFINE (ly_make_keysig_entry, "ly:make-keysig-entry",
>+   2, 3, 0, (SCM note, SCM alter, SCM octave, SCM barnumber, SCM measurepos),
>+   "An entry for key signature. Key signatures consists of lists of
these. The three last arguments are used for local key changes.")
>+{

please break string and wrap lines


>+  LY_ASSERT_TYPE (scm_is_integer, note, 1);
>+  LY_ASSERT_TYPE (scm_is_integer, alter, 2);
>+
>+  if (octave == SCM_UNDEFINED)
>+    {
>+      Key_entry e (scm_to_int (note), scm_to_int (alter));
>+
>+      return e.smobbed_copy ();
>+    }
>+  else
>+    {
>+      LY_ASSERT_TYPE (scm_is_integer, octave, 3);
>+      LY_ASSERT_TYPE (scm_is_integer, barnumber, 4);
>+      LY_ASSERT_SMOB (Moment, measurepos, 5);

can you check for unsmob_moment() here?

> /* FIXME: why is octave == 0 and default not middleC ? */
>+/* Because otherwise Pitch () would not be a "zero element" -
>+   e.g. implementation of negated () would not work. -rz ! */

Huh? I recall that octave == 0 is actually middle C. The strange thing
is that c' (with one quote) is is encoded as octave 0.



--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen


_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: accidental changes: review

Rune Zedeler
Thanks for comments.

Han-Wen Nienhuys skrev:
>> +  int octave_;
>> +  int notename_;
>> +  Rational alteration_;
>
> Why don't you use a Pitch object for this combination? You would get
> Scale* for free.

Primarily because the two values octave_ and has_octave_ are very
closely related.
They really /should/ be joined to an int option - if such a thing did
exist in c++.
Therefore I find it very errorprone to move the octave_ into another
object without has_octave_. The pitch object will not make sense if
has_octave_==false.
We have

LY_DEFINE (ly_make_keysig_entry, "ly:make-keysig-entry",
           2, 3, 0, (SCM note, SCM alter,
                     SCM octave, SCM barnumber, SCM measurepos),
           "An entry for key signature. Key signatures consists of lists of"
           "these. The three last arguments are used for local key changes.")

It will not make sense to join the three first together, because the two
first are related, and the three last are related.

>> /* FIXME: why is octave == 0 and default not middleC ? */
>> +/* Because otherwise Pitch () would not be a "zero element" -
>> +   e.g. implementation of negated () would not work. -rz ! */
>
> Huh? I recall that octave == 0 is actually middle C. The strange thing
> is that c' (with one quote) is is encoded as octave 0.

Just to ensure I understand you correctly: The FIXME is wrong?
It /should/ say
FIXME: Why does octave == 0 refer to middleC-octave and not the octave
below?

-Rune


_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: accidental changes: review

Han-Wen Nienhuys-3
2007/9/25, Rune Zedeler <[hidden email]>:

> >> +  int octave_;
> >> +  int notename_;
> >> +  Rational alteration_;
> >
> > Why don't you use a Pitch object for this combination? You would get
> > Scale* for free.
>
> Primarily because the two values octave_ and has_octave_ are very
> closely related.
> They really /should/ be joined to an int option - if such a thing did
> exist in c++.

you might want to look into making octave optional for pitch; I'm not
sure if it is worth the trouble, but it would make key signatures more
logical to specify.

Otherwise, you could use an int*


> >> /* FIXME: why is octave == 0 and default not middleC ? */
> >> +/* Because otherwise Pitch () would not be a "zero element" -
> >> +   e.g. implementation of negated () would not work. -rz ! */
> >
> > Huh? I recall that octave == 0 is actually middle C. The strange thing
> > is that c' (with one quote) is is encoded as octave 0.
>
> Just to ensure I understand you correctly: The FIXME is wrong?

I think the fixme should just go away.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen


_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: accidental changes: review

Juergen Reuter
On Tue, 25 Sep 2007, Han-Wen Nienhuys wrote:

> 2007/9/25, Rune Zedeler <[hidden email]>:
>>>> +  int octave_;
>>>> +  int notename_;
>>>> +  Rational alteration_;
>>>
>>> Why don't you use a Pitch object for this combination? You would get
>>> Scale* for free.
>>
>> Primarily because the two values octave_ and has_octave_ are very
>> closely related.
>> They really /should/ be joined to an int option - if such a thing did
>> exist in c++.
>
> you might want to look into making octave optional for pitch; I'm not
> sure if it is worth the trouble, but it would make key signatures more
> logical to specify.
>
> Otherwise, you could use an int*
>
>

I am just curious: Why not introducing a new class "Octaveless_pitch"
or "Octave_Pitch" (i.e. pitch within an octave) that contains all of
"Pitch" except for the octaves, and then making "Pitch" a subclass of
"Octaveless_pitch" that just adds the octaves to its superclass?

Greetings,
Juergen


_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: accidental changes: review

Trevor Bača-2
On 9/26/07, Juergen Reuter <[hidden email]> wrote:

> On Tue, 25 Sep 2007, Han-Wen Nienhuys wrote:
>
> > 2007/9/25, Rune Zedeler <[hidden email]>:
> >>>> +  int octave_;
> >>>> +  int notename_;
> >>>> +  Rational alteration_;
> >>>
> >>> Why don't you use a Pitch object for this combination? You would get
> >>> Scale* for free.
> >>
> >> Primarily because the two values octave_ and has_octave_ are very
> >> closely related.
> >> They really /should/ be joined to an int option - if such a thing did
> >> exist in c++.
> >
> > you might want to look into making octave optional for pitch; I'm not
> > sure if it is worth the trouble, but it would make key signatures more
> > logical to specify.
> >
> > Otherwise, you could use an int*
> >
> >
>
> I am just curious: Why not introducing a new class "Octaveless_pitch"
> or "Octave_Pitch" (i.e. pitch within an octave) that contains all of
> "Pitch" except for the octaves, and then making "Pitch" a subclass of
> "Octaveless_pitch" that just adds the octaves to its superclass?
[The (American) academic terms for these are "pitch-class" for the
octaveless notenames and "pitch" for actual pitches specified in a
certain octave. Robert Morris's "Composition with Pitch-classes"
[1987; Yale] is a nice example. So here, Pitch_class would be the
superclass from which Pitch would inherit.]



--
Trevor Bača
[hidden email]

_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: accidental changes: review

Han-Wen Nienhuys-3
In reply to this post by Juergen Reuter
2007/9/26, Juergen Reuter <[hidden email]>:

> I am just curious: Why not introducing a new class "Octaveless_pitch"
> or "Octave_Pitch" (i.e. pitch within an octave) that contains all of
> "Pitch" except for the octaves, and then making "Pitch" a subclass of
> "Octaveless_pitch" that just adds the octaves to its superclass?

There would only be one scheme type, the one for the base class, so
this will require some extra typechecking for the Scheme interfaces

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen


_______________________________________________
lilypond-devel mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/lilypond-devel