accidental changes: review

[Han-Wen Nienhuys]

>-      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 - address@hidden - http://www.xs4all.nl/~hanwen