Issue 5284: improve ASSIGN_EVENT_ONCE (issue 338540043 by address@hidden)

[nine . fierce . ballads]

Reviewers: ,

Description:
https://sourceforge.net/p/testlilyissues/issues/5284/

* Really assign the event only once.
* End an unnecessary relationship with translator listeners.
* Rephrase the warning so that it could be used more broadly than for
  simultaneous events.
* Rephrase the warning so that it could be used with events of
  different classes (e.g. abc-mark-event and xyz-mark-event).
* Inline the common case that the event is assigned without warning.

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

Affected files (+53, -45 lines):
  M input/regression/warn-conflicting-key-signatures.ly
  M lily/include/stream-event.hh
  M lily/include/translator.hh
  M lily/stream-event.cc
  M lily/translator.cc


Index: input/regression/warn-conflicting-key-signatures.ly
diff --git a/input/regression/warn-conflicting-key-signatures.ly  
b/input/regression/warn-conflicting-key-signatures.ly
index  
7d805072a78c089af89bc54d7e7c29cf11c631bc..eb228ebe484ab2f2d42fd5d98f0287488a686e01  
100644
--- a/input/regression/warn-conflicting-key-signatures.ly
+++ b/input/regression/warn-conflicting-key-signatures.ly
@@ -1,7 +1,7 @@
 \version "2.16.0"
 #(ly:set-option 'warning-as-error #f)
-#(ly:expect-warning (ly:translate-cpp-warning-scheme "Two simultaneous %s  
events, junking this one") "key-change")
-#(ly:expect-warning (ly:translate-cpp-warning-scheme "Previous %s event  
here") "key-change")
+#(ly:expect-warning (ly:translate-cpp-warning-scheme "Junking this %s  
event ...") "key-change")
+#(ly:expect-warning (ly:translate-cpp-warning-scheme "... because it  
conflicts with this %s event") "key-change")

 \header {
   texidoc = "If you specify two different key signatures at one point, a
@@ -9,7 +9,7 @@ warning is printed."

 }

-\score {
+\score {
 \context Voice <<
  { \key cis \major cis4 \key bes \major bes4 }
  { \key cis \major fis4 \key es \major g4 }
Index: lily/include/stream-event.hh
diff --git a/lily/include/stream-event.hh b/lily/include/stream-event.hh
index  
d554f584dcec8eaaeb34556edc1a465a1e0784cf..927e390b2331206cd8cefe97768944a08df5a0df  
100644
--- a/lily/include/stream-event.hh
+++ b/lily/include/stream-event.hh
@@ -48,4 +48,28 @@ public:

 SCM ly_event_deep_copy (SCM);

+extern void
+warn_reassign_event_ptr (Stream_event &old_ev, Stream_event *new_ev);
+
+// Assign to old_ev at most once, returning true when it happens, and  
warning
+// (with some exceptions) about attempted reassignments.
+inline bool
+assign_event_ptr_once (Stream_event *&old_ev, Stream_event *new_ev)
+{
+  if (!old_ev)
+    {
+      old_ev = new_ev;
+      return old_ev;
+    }
+  else
+    {
+      warn_reassign_event_ptr (*old_ev, new_ev);
+      return false;
+    }
+}
+
+// This macro is no longer necessary.
+// If you have time to eliminate it, go ahead.
+#define ASSIGN_EVENT_ONCE(o,n) assign_event_ptr_once (o, n)
+
 #endif /* STREAM_EVENT_HH */
Index: lily/include/translator.hh
diff --git a/lily/include/translator.hh b/lily/include/translator.hh
index  
91bd59fe4aef5c72efe334a9687a04dd2d1626ad..33acd337f137e1777b5c2f8e79b2b2cebc4db6bb  
100644
--- a/lily/include/translator.hh
+++ b/lily/include/translator.hh
@@ -224,10 +224,4 @@ SCM get_translator_creator (SCM s);
 Moment get_event_length (Stream_event *s, Moment now);
 Moment get_event_length (Stream_event *s);

-/*
-  This helper is only meaningful inside listen_* methods.
-*/
-extern bool internal_event_assignment (Stream_event **old_ev, Stream_event  
*new_ev, const char *function);
-#define ASSIGN_EVENT_ONCE(o,n) internal_event_assignment (&o, n,  
__FUNCTION__)
-
 #endif // TRANSLATOR_HH
Index: lily/stream-event.cc
diff --git a/lily/stream-event.cc b/lily/stream-event.cc
index  
64c9a979d9950907dde19ffbb0a49978a5a21f12..16a230f44d249633205d15c004f7338b889ab0e7  
100644
--- a/lily/stream-event.cc
+++ b/lily/stream-event.cc
@@ -21,8 +21,10 @@

 #include "context.hh"
 #include "input.hh"
+#include "international.hh"
 #include "music.hh"
 #include "pitch.hh"
+#include <string>

 /* TODO: Rename Stream_event -> Event */

@@ -113,3 +115,27 @@ Stream_event::undump (SCM data)
   obj->mutable_property_alist_ = scm_reverse (scm_cdr (data));
   return obj->unprotect ();
 }
+
+void
+warn_reassign_event_ptr (Stream_event &old_ev, Stream_event *new_ev)
+{
+  if (!new_ev) // not expected
+    return;
+
+  if (to_boolean (scm_equal_p (old_ev.self_scm (), new_ev->self_scm ())))
+    return; // nothing of value was lost
+
+  std::string nc = ly_symbol2string (scm_car (new_ev->get_property  
("class")));
+  std::string oc = ly_symbol2string (scm_car (old_ev.get_property  
("class")));
+
+  // remove the suffix "-event" if it is present
+  const std::string suffix("-event");
+  if (0 == nc.compare(nc.size() - suffix.size(), suffix.size(), suffix))
+    nc.erase(nc.size() - suffix.size());
+  if (0 == oc.compare(oc.size() - suffix.size(), suffix.size(), suffix))
+    oc.erase(oc.size() - suffix.size());
+
+  new_ev->origin ()->warning (_f ("Junking this %s event ...", nc.c_str  
()));
+  old_ev.origin ()->warning (_f ("... because it conflicts with this %s  
event",
+                                 oc.c_str ()));
+}
Index: lily/translator.cc
diff --git a/lily/translator.cc b/lily/translator.cc
index  
e57e1c391af53eec7dc93253da33fe28d5cff8e7..9f489102d5a0251a115fe0ea3606b3836cb9fd6b  
100644
--- a/lily/translator.cc
+++ b/lily/translator.cc
@@ -22,7 +22,6 @@
 #include "context-def.hh"
 #include "dispatcher.hh"
 #include "global-context.hh"
-#include "international.hh"
 #include "translator-group.hh"
 #include "warn.hh"

@@ -266,39 +265,4 @@ get_event_length (Stream_event *e, Moment now)
   return len;
 }

-/*
-  Helper, used through ASSIGN_EVENT_ONCE to throw warnings for
-  simultaneous events. The helper is only useful in listen_* methods
-  of translators.
-*/
-bool
-internal_event_assignment (Stream_event **old_ev, Stream_event *new_ev,  
const char *function)
-{
-  if (*old_ev
-      && !to_boolean (scm_equal_p ((*old_ev)->self_scm (),
-                                   new_ev->self_scm ())))
-    {
-      /* extract event class from function name */
-      string ev_class = function;
-
-      /* This assertion fails if EVENT_ASSIGNMENT was called outside a
-         translator listener. Don't do that. */
-      const char *prefix = "listen_";
-      assert (0 == ev_class.find (prefix));
-
-      /* "listen_foo_bar" -> "foo-bar" */
-      ev_class.erase (0, strlen (prefix));
-      replace_all (&ev_class, '_', '-');
-
-      new_ev->origin ()->warning (_f ("Two simultaneous %s events, junking  
this one", ev_class.c_str ()));
-      (*old_ev)->origin ()->warning (_f ("Previous %s event here",  
ev_class.c_str ()));
-      return false;
-    }
-  else
-    {
-      *old_ev = new_ev;
-      return true;
-    }
-}
-
 // Base class.  Not instantiated.  No ADD_TRANSLATOR call.