Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by address@hidden)

[nine . fierce . ballads]

LGTM but there are a couple of things I strongly encourage considering.


https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc
File lily/global-context.cc (right):

https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc#newcode49
lily/global-context.cc:49: event_source ()->add_listener (GET_LISTENER
(static_cast<Context *> (this), create_context_from_event),
This is a wart.  Without the cast, my version of g++ has this to say:

... error: no matching function for call to
'Callback_wrapper::make_smob<trampoline<std::decay<Global_context&>::type,
&Context::create_context_from_event> > ...

Notice that it correctly selects the method
Context::create_context_from_event.  It should be possible to extract
the proper class type in a second step from that instead of directly
from the provided pointer.

For that, I think you might need to write a template function that
returns the class type given the member function pointer as a function
parameter; I don't remember if the C++11 standard library has this, and
I don't see it as I skim through cppreference.

I'm sure you won't like complicating your macro, but as it stands,
someone who writes code that is missing a cast has some reading to do,
whereas if you complicate the macro to handle this case, they won't have
to read it.

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

https://codereview.appspot.com/549890043/diff/573710043/lily/include/listener.hh#newcode143
lily/include/listener.hh:143: // This is somewhat more official, but way
overkill
Overkill in what respect?  I would use standard features where they are
appropriate rather than reinventing them.  Especially in tricky
situations like this, using something with a fixed meaning is
beneficial.

It looks like dummy_deref also removes const from the class type, which
would be more like std::remove_const<std::remove_reference<...

If you must have something shorter, consider
std::decay<decltype(*(ptr))>::type.  It removes the reference and
const/volatile.  It also tries a couple more conversions than this
relies on, but not ones that seem likely to cause stumbling.

In any case, as a not-fully-oriented contributor, I prefer not finding
stuff in #if 0 when I use grep.  If you're set on keeping dummy_deref,
it would be enough just to comment that it's an alternative to
std::remove_const plus std::remove_reference.

But this might all be moot, depending on your reaction to my comment on
the static_cast in Global_context.

https://codereview.appspot.com/549890043/