Re: Prob::equal_p: discard "origin" property (issue 363740043 by address@hidden)

[dak]

Reviewers: thomasmorley651,


https://codereview.appspot.com/363740043/diff/1/lily/prob.cc
File lily/prob.cc (right):

https://codereview.appspot.com/363740043/diff/1/lily/prob.cc#newcode36
lily/prob.cc:36: equality; e.g., that two probs are equal iff they can
be
On 2018/07/27 09:32:55, thomasmorley651 wrote:
> Is this comment correct?
> Shouldn't it be
> "...iff they _can't_ be distinguished..."

> Please ignore, if it's a stupid remark, caused by my lack of
knowledge...

Well, in this case it is rather the comment that is stupid, but not due
to a lack of knowledge.  I'll fix this silently in the commit touching
this file.  No point in another review just for it.

Description:
Prob::equal_p: discard "origin" property

Previously elements of class Input was considered equal when compared
as part of Music property lists but this did not take into account
that some music might store its origin (if any) at different locations
in its property lists.  So we just discard any "origin" property
completely, regardless of position in the property list and its actual
value type.


Also contains commit:

Don't set origin on copied music explicitly

There would be some mild incentive if the origin were accessed an
inordinary amount of times (and thus leaving it unset would maximize
access times to it) but there is no evidence for that.  One reason
might be to ensure greater structural similarity between copies of
music, but Prob::equal_p has been changed to be robust against
differences here.

Please review this at https://codereview.appspot.com/363740043/

Affected files (+21, -13 lines):
  M lily/music.cc
  M lily/prob.cc


Index: lily/music.cc
diff --git a/lily/music.cc b/lily/music.cc
index  
cbae4916b6542db8195d41599b37d17d89cfeab4..c70ce07c69aaa33bf80ea5d476905b22262e2d69  
100644
--- a/lily/music.cc
+++ b/lily/music.cc
@@ -85,9 +85,6 @@ Music::Music (Music const &m)
 {
   length_callback_ = m.length_callback_;
   start_callback_ = m.start_callback_;
-
-  /// why?
-  set_spot (*m.origin ());
 }

 Moment
Index: lily/prob.cc
diff --git a/lily/prob.cc b/lily/prob.cc
index  
d05a33e4f3aa60387ad40a612deaf01a6d952a54..8c7bd47dac698931e8b4b80e45d063a530d8aaa9  
100644
--- a/lily/prob.cc
+++ b/lily/prob.cc
@@ -55,23 +55,34 @@ Prob::equal_p (SCM sa, SCM sb)
       SCM aprop = props[0][i];
       SCM bprop = props[1][i];

-      for (;
-           scm_is_pair (aprop) && scm_is_pair (bprop);
-           aprop = scm_cdr (aprop), bprop = scm_cdr (bprop))
+      for (;; aprop = scm_cdr (aprop), bprop = scm_cdr (bprop))
         {
+          SCM origin_sym = ly_symbol2scm ("origin");
+          // Skip over origin fields
+          while (scm_is_pair (aprop)
+                 && scm_is_eq (origin_sym, scm_caar (aprop)))
+            aprop = scm_cdr (aprop);
+
+          while (scm_is_pair (bprop)
+                 && scm_is_eq (origin_sym, scm_caar (bprop)))
+            bprop = scm_cdr (bprop);
+
+          /* is one list shorter? */
+          if (!scm_is_pair (aprop))
+            if (!scm_is_pair (bprop))
+              break;
+            else
+              return SCM_BOOL_F;
+          else if (!scm_is_pair (bprop))
+            return SCM_BOOL_F;
+
           SCM aval = scm_cdar (aprop);
           SCM bval = scm_cdar (bprop);
           if (!scm_is_eq (scm_caar (aprop), scm_caar (bprop))
-              || (!(unsmob<Input> (aval) && unsmob<Input> (bval))
-                  && !ly_is_equal (aval, bval)))
+              || !ly_is_equal (aval, bval))
             return SCM_BOOL_F;
         }
-
-      /* is one list shorter? */
-      if (!scm_is_null (aprop) || !scm_is_null (bprop))
-        return SCM_BOOL_F;
     }
-
   return SCM_BOOL_T;
 }