Re: Implement Grob::event_cause, Grob::ultimate_event_cause (issue 559500043 by address@hidden)

[dak]

Reviewers: hahnjo, hanwenn, Dan Eble,


https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc#newcode733
lily/grob.cc:733: while (unsmob<Grob> (cause))
On 2020/02/21 00:13:25, Dan Eble wrote:
> I appreciate that issuing warnings is not performance-sensitive, and
that you
> simply transplanted this code, but what do you think about avoiding
repeating
> the unsmob, to set a good example?  Something like ...
> 
>     while (const Grob *g = unsmob<Grob> (cause))
>       {
>         cause = g->get_property ("cause");
>       }

Not going to use const here since it is pointless while we have nothing
like const_unsmob, and would not use that order anyway since I consider
it misleading (it suggests a declaration of a constant, rather than a
pointer to a constant).  Other than that, fine.

Description:
Implement Grob::event_cause, Grob::ultimate_event_cause

Those were implemented in Grob_info only previously which does not make
a lot of sense,
given that they don't access anything particular to Grob_info.


Also contains commit:

Use Grob::event_cause and Grob::ultimate_event_cause where useful



Frankly, this one has been bugging me on and off for years.  I just
had one compilation error too much today.

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

Affected files (+42, -37 lines):
  M lily/accidental-engraver.cc
  M lily/accidental-placement.cc
  M lily/grob.cc
  M lily/grob-info.cc
  M lily/include/grob.hh
  M lily/lyric-engraver.cc
  M lily/percent-repeat-item.cc
  M lily/slur-engraver.cc
  M lily/tie-engraver.cc


Index: lily/accidental-engraver.cc
diff --git a/lily/accidental-engraver.cc b/lily/accidental-engraver.cc
index 
f15981ebb4f3d79f8f16ca073ef1c51935303413..877ec1cc5ba0c21d96fb808ab20d11d19741e06c
 100644
--- a/lily/accidental-engraver.cc
+++ b/lily/accidental-engraver.cc
@@ -374,8 +374,8 @@ Accidental_engraver::stop_translation_timestep ()
         {
           // Don't mark accidentals as "tied" when the pitch is not
           // actually the same.  This is relevant for enharmonic ties.
-          Stream_event *le = unsmob<Stream_event> (l->get_property ("cause"));
-          Stream_event *re = unsmob<Stream_event> (r->get_property ("cause"));
+          Stream_event *le = l->event_cause ();
+          Stream_event *re = r->event_cause ();
           if (le && re
               && !ly_is_equal (le->get_property ("pitch"), re->get_property 
("pitch")))
             continue;
Index: lily/accidental-placement.cc
diff --git a/lily/accidental-placement.cc b/lily/accidental-placement.cc
index 
7965fcb4b9613399eca830f44a9087aa5709f1ee..e34bfb85293b1c55712d8fd3ac175d2169c2603c
 100644
--- a/lily/accidental-placement.cc
+++ b/lily/accidental-placement.cc
@@ -39,9 +39,8 @@ using std::vector;
 static Pitch *
 accidental_pitch (Grob *acc)
 {
-  SCM cause = acc->get_parent (Y_AXIS)->get_property ("cause");
+  Stream_event *mcause = acc->get_parent (Y_AXIS)->event_cause ();
 
-  Stream_event *mcause = unsmob<Stream_event> (cause);
   if (!mcause)
     {
       programming_error ("note head has no event cause");
Index: lily/grob-info.cc
diff --git a/lily/grob-info.cc b/lily/grob-info.cc
index 
70e2ef55b4b966019f8b35e9f437d6637eea6ef0..9b1c265db31ceba86eb00caef323dda4b75319ff
 100644
--- a/lily/grob-info.cc
+++ b/lily/grob-info.cc
@@ -46,8 +46,7 @@ Grob_info::Grob_info ()
 Stream_event *
 Grob_info::event_cause () const
 {
-  SCM cause = grob_->get_property ("cause");
-  return unsmob<Stream_event> (cause);
+  return grob_->event_cause ();
 }
 
 Context *
@@ -59,12 +58,7 @@ Grob_info::context () const
 Stream_event *
 Grob_info::ultimate_event_cause () const
 {
-  SCM cause = grob_->self_scm ();
-  while (unsmob<Grob> (cause))
-    {
-      cause = unsmob<Grob> (cause)->get_property ("cause");
-    }
-  return unsmob<Stream_event> (cause);
+  return grob_->ultimate_event_cause ();
 }
 
 bool
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index 
a17869b00adf97f330d16ee23c887132899c780c..c4646b96b59d7bd6bb277e20438d19eef14bb126
 100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -716,20 +716,36 @@ Grob::internal_vertical_less (Grob *g1, Grob *g2, bool 
pure)
   return false;
 }
 
+/****************************************************************
+  CAUSES
+****************************************************************/
+Stream_event *
+Grob::event_cause () const
+{
+  SCM cause = get_property ("cause");
+  return unsmob<Stream_event> (cause);
+}
+
+Stream_event *
+Grob::ultimate_event_cause () const
+{
+  SCM cause = self_scm ();
+  while (unsmob<Grob> (cause))
+    {
+      cause = unsmob<Grob> (cause)->get_property ("cause");
+    }
+  return unsmob<Stream_event> (cause);
+}
+
+
+
 /****************************************************************
   MESSAGES
 ****************************************************************/
 void
 Grob::programming_error (const string &s) const
 {
-  SCM cause = self_scm ();
-  while (Grob *g = unsmob<Grob> (cause))
-    cause = g->get_property ("cause");
-
-  /* ES TODO: cause can't be Music*/
-  if (Music *m = unsmob<Music> (cause))
-    m->origin ()->programming_error (s);
-  else if (Stream_event *ev = unsmob<Stream_event> (cause))
+  if (Stream_event *ev = ultimate_event_cause ())
     ev->origin ()->programming_error (s);
   else
     ::programming_error (s);
@@ -738,14 +754,7 @@ Grob::programming_error (const string &s) const
 void
 Grob::warning (const string &s) const
 {
-  SCM cause = self_scm ();
-  while (Grob *g = unsmob<Grob> (cause))
-    cause = g->get_property ("cause");
-
-  /* ES TODO: cause can't be Music*/
-  if (Music *m = unsmob<Music> (cause))
-    m->origin ()->warning (s);
-  else if (Stream_event *ev = unsmob<Stream_event> (cause))
+  if (Stream_event *ev = ultimate_event_cause ())
     ev->origin ()->warning (s);
   else
     ::warning (s);
Index: lily/include/grob.hh
diff --git a/lily/include/grob.hh b/lily/include/grob.hh
index 
d0195610af9f97634057333b595de0e9445783f4..d01fdf8169912cdf8eda02b21eabfbad4b6f348e
 100644
--- a/lily/include/grob.hh
+++ b/lily/include/grob.hh
@@ -24,6 +24,7 @@
 #include "virtual-methods.hh"
 #include "dimension-cache.hh"
 #include "grob-interface.hh"
+#include "lily-proto.hh"
 
 #include <set>
 
@@ -117,6 +118,10 @@ public:
   void instrumented_set_property (SCM, SCM, char const *, int, char const *);
   void internal_set_property (SCM sym, SCM val);
 
+  /* causes */
+  Stream_event *event_cause () const;
+  Stream_event *ultimate_event_cause () const;
+
   /* messages */
   void warning (const std::string&) const;
   void programming_error (const std::string&) const;
Index: lily/lyric-engraver.cc
diff --git a/lily/lyric-engraver.cc b/lily/lyric-engraver.cc
index 
6cc5cff89cc2292a3d0088f109f352691d285bfc..1de70bf1ba6cbebc7b5359ea67b5e51231153d3c
 100644
--- a/lily/lyric-engraver.cc
+++ b/lily/lyric-engraver.cc
@@ -146,7 +146,7 @@ get_current_note_head (Context *voice)
       // It's a bit irritating that we just have the length and
       // duration of the Grob.
       Moment end_from_now =
-        get_event_length (unsmob<Stream_event> (g->get_property ("cause")), 
now)
+        get_event_length (g->event_cause (), now)
         + now;
       // We cannot actually include more than a single grace note
       // using busyGrobs on ungraced lyrics since a grob ending on
Index: lily/percent-repeat-item.cc
diff --git a/lily/percent-repeat-item.cc b/lily/percent-repeat-item.cc
index 
177266693f6a8ecc2c9b9c4c6c4c1527b0478810..776a8b62a000d2dee3c3f71f35ef2867db1f2620
 100644
--- a/lily/percent-repeat-item.cc
+++ b/lily/percent-repeat-item.cc
@@ -80,7 +80,7 @@ SCM
 Percent_repeat_item_interface::beat_slash (SCM grob)
 {
   Grob *me = unsmob<Grob> (grob);
-  Stream_event *cause = unsmob<Stream_event> (me->get_property ("cause"));
+  Stream_event *cause = me->event_cause ();
   int count = robust_scm2int (cause->get_property ("slash-count"), 1);
 
   Stencil m;
Index: lily/slur-engraver.cc
diff --git a/lily/slur-engraver.cc b/lily/slur-engraver.cc
index 
8498ebf2abc2002613f4e9b09c745b6795b7b70d..731abc69f8dfd88c398ce516eb2ce6f21e0a9d3a
 100644
--- a/lily/slur-engraver.cc
+++ b/lily/slur-engraver.cc
@@ -151,8 +151,7 @@ Slur_engraver::acknowledge_note_column (Grob_info info)
   extract_grob_set (e, "note-heads", heads);
   for (vsize i = heads.size (); i--;)
     {
-      if (Stream_event *ev =
-          unsmob<Stream_event> (heads[i]->get_property ("cause")))
+      if (Stream_event *ev = heads[i]->event_cause ())
         for (LEFT_and_RIGHT (d))
           {
             std::pair<Note_slurs::const_iterator, Note_slurs::const_iterator> 
its
@@ -249,7 +248,7 @@ Slur_engraver::can_create_slur (SCM id, vsize old_slurs, 
vsize *event_idx, Strea
           if (!updown)
             return false;
 
-          Stream_event *c = unsmob<Stream_event> (slur->get_property 
("cause"));
+          Stream_event *c = slur->event_cause ();
 
           if (!c)
             {
Index: lily/tie-engraver.cc
diff --git a/lily/tie-engraver.cc b/lily/tie-engraver.cc
index 
f31eb177f3e98115fd7811ee91a4d606049ac71e..f05b866e4c6ba3960892683afaff8254db156197
 100644
--- a/lily/tie-engraver.cc
+++ b/lily/tie-engraver.cc
@@ -157,8 +157,8 @@ Tie_engraver::tie_notehead (Grob *h, bool enharmonic)
   for (vsize i = 0; i < heads_to_tie_.size (); i++)
     {
       Grob *th = heads_to_tie_[i].head_;
-      Stream_event *right_ev = unsmob<Stream_event> (h->get_property 
("cause"));
-      Stream_event *left_ev = unsmob<Stream_event> (th->get_property 
("cause"));
+      Stream_event *right_ev = h->event_cause ();
+      Stream_event *left_ev = th->event_cause ();
 
       /*
         maybe should check positions too.
@@ -282,8 +282,7 @@ Tie_engraver::process_acknowledged ()
   for (vsize i = 0; i < now_heads_.size (); i++)
     {
       Grob *head = now_heads_[i];
-      Stream_event *left_ev
-        = unsmob<Stream_event> (head->get_property ("cause"));
+      Stream_event *left_ev = head->event_cause ();
 
       if (!left_ev)
         {