Re: Redesign listeners using templates (issue 235790043 by address@hidden)

[dak]

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#oldcode84
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#newcode111
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#newcode153
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#newcode164
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.

https://codereview.appspot.com/235790043/