Redesign listeners using templates

Redesign listeners using templates

This is actually just a start since it does not touch most of the
infrastructure for translators.


Contains the following commits in reverse order:



Remove Scheme listeners as they are just callbacks now.


Reimplement Listener around generic SCM callback and instance


Run preparatory script replacing listener macros

This script is

for i in $(git grep -l 'IMPLEMENT_LISTENER (')
do
    sed -i '/IMPLEMENT_LISTENER ([^)]*$/{N;s/\n\s*/ /}' $i
    sedscript=$(git grep -h 'IMPLEMENT_LISTENER (' $i|
		       sed 's|^.*IMPLEMENT_LISTENER (\([^,]*\), \([^)]*\)).*$|s/GET_LISTENER (\\([_a-z]\\+->\\)\\?\2)/\\1GET_LISTENER (\1, \2)/|')
    sed -i "$sedscript" $i
done
sed -i '/IMPLEMENT_LISTENER (/d' $(git grep -l 'IMPLEMENT_LISTENER (')
sed -i 's/DECLARE_LISTENER (\([^)]*\));\?/void \1 (SCM);/' $(git grep -l 'DECLARE_LISTENER (')

Make dispatchers callable for dispatching


Move self_scm_ into non-templated Smob_core base class.

[benko.pal, 2015-04-30 18:48:41]

looks exciting to me; my only comment is about a comment

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:27: register a method as an event handler in a
dispatcher, then you
this whole comment is probably to be deleted; IIUC the my_listen example below
replaces it.

[dak, 2015-04-30 19:14:37]

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:27: register a method as an event handler in a
dispatcher, then you
On 2015/04/30 18:48:41, benko.pal wrote:
> this whole comment is probably to be deleted; IIUC the my_listen example below
> replaces it.

Uhm.  That's what happens if you convert the documentation along with the actual
code using sed scripts.  The contained examples are pretty much ok but no longer
fit the description.  I'll rework this comment.

[dak, 2015-05-01 00:20:06]

Improve comment, cosmetic change

[Dan Eble, 2015-05-01 00:22:09]

I have reviewed the changes to smob and listener.

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (left):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#oldc...
lily/include/listener.hh:84: Listener (Listener const &other);
So you're OK with the compiler-generated copy constructor?  (Just checking.)

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:111: return *Listener::unsmob (a) == *Listener::unsmob
(b)
If there are no concerns about dereferencing null pointers, a short comment
would be comforting.

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:153: T *t = dynamic_cast<T*> (T::unsmob (target));
This is not a problem with your change, but I notice that using dynamic_cast
after unsmob is very common.  This looks like a candidate for future
simplification something along these lines:

template <class T>
static inline T* ly_unsmob(SCM s)
{
    return dynamic_cast<T*>(T::unsmob(s));
}

invoked as T* t = ly_unsmob<T>(target);

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:164: unsmob (self)->trampoline_ (target, ev);
I suppose that here, unsmob (self) will always succeed because guile would not
have found this procedure if the type were wrong?

[dak, 2015-05-01 00:44:45]

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (left):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#oldc...
lily/include/listener.hh:84: Listener (Listener const &other);
On 2015/05/01 00:22:09, Dan Eble wrote:
> So you're OK with the compiler-generated copy constructor?  (Just checking.)

I don't mind either way.  You could assign Listeners just fine, whether
smobbified or not, and smobify the copy.  The actual per-instance data is just
two SCM values.

I don't think this is used anywhere but it would work if one wanted to.

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:111: return *Listener::unsmob (a) == *Listener::unsmob
(b)
On 2015/05/01 00:22:08, Dan Eble wrote:
> If there are no concerns about dereferencing null pointers, a short comment
> would be comforting.

There is no concern since equal_p is called via the smob-specific callback
function table when both of the arguments of equal? are of the same smob type.

So this function can never be called with a and b being anything but a valid
Listener smob unless you were to call it manually.  And a valid Listener smob
will unsmob to a valid pointer to the corresponding Listener structure.

I am not going to put in an explanation for every definition of equal_p for some
smob class.

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:153: T *t = dynamic_cast<T*> (T::unsmob (target));
On 2015/05/01 00:22:09, Dan Eble wrote:
> This is not a problem with your change, but I notice that using dynamic_cast
> after unsmob is very common.  This looks like a candidate for future
> simplification something along these lines:
> 
> template <class T>
> static inline T* ly_unsmob(SCM s)
> {
>     return dynamic_cast<T*>(T::unsmob(s));
> }
> 
> invoked as T* t = ly_unsmob<T>(target);

I don't think that warrants a template function of its own.  It just makes
matters murkier without saving a significant amount of typing.

The following SCM_ASSERT_TYPE, however, screams for a dedicated macro that will
be less noisy and cryptic in the source code and will hopefully output better
type information.

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newc...
lily/include/listener.hh:164: unsmob (self)->trampoline_ (target, ev);
On 2015/05/01 00:22:09, Dan Eble wrote:
> I suppose that here, unsmob (self) will always succeed because guile would not
> have found this procedure if the type were wrong?

Yes, this is again going through GUILE's smob callback tables.

[Dan Eble, 2015-05-01 01:22:57]

On 2015/05/01 00:44:45, dak wrote:
> I am not going to put in an explanation for every definition of equal_p for
some
> smob class.

OK, that makes sense now.
 
> > invoked as T* t = ly_unsmob<T>(target);
> 
> I don't think that warrants a template function of its own.  It just makes
> matters murkier without saving a significant amount of typing.

Then we'll differ. Repeating the type three times on a line is twice more than I
like. (With C++11 it could be auto t = ly_unsmob<T>(target).)

But deferring to your judgment, what is your opinion of

   static Engraver *unsmob (SCM eng) {
       return dynamic_cast<Engraver *> (Translator::unsmob (eng));
   }

and eight similar cases I can find which go about saving a small amount of
typing independently?

[dak, 2015-05-01 07:14:15]

On 2015/05/01 01:22:57, Dan Eble wrote:
> On 2015/05/01 00:44:45, dak wrote:
> > I am not going to put in an explanation for every definition of equal_p for
> some
> > smob class.
> 
> OK, that makes sense now.
>  
> > > invoked as T* t = ly_unsmob<T>(target);
> > 
> > I don't think that warrants a template function of its own.  It just makes
> > matters murkier without saving a significant amount of typing.
> 
> Then we'll differ. Repeating the type three times on a line is twice more than
I
> like. (With C++11 it could be auto t = ly_unsmob<T>(target).)
> 
> But deferring to your judgment, what is your opinion of
> 
>    static Engraver *unsmob (SCM eng) {
>        return dynamic_cast<Engraver *> (Translator::unsmob (eng));
>    }
> 
> and eight similar cases I can find which go about saving a small amount of
> typing independently?

I am not particularly enamored.  The expectation would be that this goes with an
is_smob, and the expectation of an is_smob is that it goes with a Scheme
predicate, and a Scheme predicate should go with a printable type (and that's a
somewhat high granularity and I don't even know how to come up with a good
naming scheme for what amounts to type names and descriptions for _every_
_single_ C++ engraver or performer).  That's quite more than 8.

Now if all that were done, the passage in question here would look nicer, and
its error message would likely be more on-spot.  But it seems like a somewhat
tricky endeavor.  I doubt it is worth the trouble.

[dak, 2015-05-01 08:16:27]

Fix naming conflict

[benko.pal, 2015-05-01 16:21:48]

LGTM

[dak, 2015-05-02 11:17:36]

Rebase on current master.