[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Redesign listeners using templates (issue 235790043 by address@hidde
From: |
nine . fierce . ballads |
Subject: |
Re: Redesign listeners using templates (issue 235790043 by address@hidden) |
Date: |
Fri, 01 May 2015 00:22:08 +0000 |
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#oldcode84
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#newcode111
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#newcode153
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#newcode164
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?
https://codereview.appspot.com/235790043/