Make GET_LISTENER take a pointer arg and deduce its type

Make GET_LISTENER take a pointer arg and deduce its type

Instead of

    Context *p;
    [...]
    Listener l = p->GET_LISTENER (Context, somecallback);

this now uses

    Context *p;
    [...]
    Listener l = GET_LISTENER (p, somecallback);

This removes the necessity for providing the somewhat awkward type
argument.

[dak, 2020-04-18 00:04:41]

formatting

[lemzwerg, 2020-04-18 05:40:53]

LGTM - but I don't know the internals well enough to really decide that.

What about using two macros instead of one?  The first would take a context as a
first argument (as it does now), the second one uses 'this' in the macro body,
omitting that as a macro argument.  The former seems to be a rarer case than the
latter.

[dak, 2020-04-18 10:18:07]

Remove ties of listener.hh to smobs.hh, reducing dependencies

[dak, 2020-04-18 10:26:01]

On 2020/04/18 05:40:53, lemzwerg wrote:
> LGTM - but I don't know the internals well enough to really decide that.

It's really not as much about the internals rather than about how to express
their use.  GET_LISTENER provides an SCM-callable memoized function port into a
C++ member function.  The last iteration of this code in issue 4357 had to add
the type argument to the GET_LISTENER macro and I was comparatively unhappy
about the resulting syntax.  The change was akin to

-  add_listener (GET_LISTENER (new_context->unset_property_from_event),
+  add_listener (new_context->GET_LISTENER (Context, unset_property_from_event),

As you can see, I had to split the previous single argument into two and
separately specify the type of the first.  The items that this technique needs
to process are
Context, &Context::unset_property_from_event for creating an SCM-accessible
memoized trampoline function and new_context for generating an SCM listener port
to a specific Smob structure.
> 
> What about using two macros instead of one?  The first would take a context as
a
> first argument (as it does now), the second one uses 'this' in the macro body,
> omitting that as a macro argument.  The former seems to be a rarer case than
the
> latter.

Well, the problem is that having GET_LISTENER (... and GET_LISTENER (this, ...
does not really take significantly more space than GET_LISTENER (... and
GET_SELF_LISTENER (... while being less clear about what is happening.

Similarly for, say, GET_OUTSIDE_LISTENER vs GET_LISTENER (if you try to give the
explicit listener the longer name).

I agree with the sentiment, but I fail to come up with a naming choice that does
not make the cure worse than the problem.

[hanwenn, 2020-04-18 10:30:27]

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

https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener...
lily/include/listener.hh:140: #define GET_LISTENER(cl, proc) get_listener
(Callback_wrapper::make_smob<Listener::trampoline<cl, &cl::proc> > ())
IMO, the problem is that the macro scopes 'proc' to come from 'cl', which is
magical. Couldn't we change the calling convention to be 

  ctx->GET_LISTENER(&SomeContext::func) 

if we call this from within a SomeContext method itself, it could be 

   GET_LISTENER(&func)

If so, would we need a macro at all?

[lemzwerg, 2020-04-18 10:32:50]

> I agree with the sentiment, but I fail to come up
> with a naming choice that does not make the cure
> worse than the problem.

I would simply use GET_LISTENERX (or GET_LISTENER1) for the longer call.

[dak, 2020-04-18 10:43:47]

On 2020/04/18 10:26:01, dak wrote:
> On 2020/04/18 05:40:53, lemzwerg wrote:
> 
> > What about using two macros instead of one?  The first would take a context
as a
> > first argument (as it does now), the second one uses 'this' in the macro
body,
> > omitting that as a macro argument.  The former seems to be a rarer case than
the
> > latter.
> 
> Well, the problem is that having GET_LISTENER (... and GET_LISTENER (this, ...
> does not really take significantly more space than GET_LISTENER (... and
> GET_SELF_LISTENER (... while being less clear about what is happening.
> 
> Similarly for, say, GET_OUTSIDE_LISTENER vs GET_LISTENER (if you try to give
the
> explicit listener the longer name).
> 
> I agree with the sentiment, but I fail to come up with a naming choice that
does
> not make the cure worse than the problem.

By the way, the last code iteration has removed specific listener support from
the Smob data type.  That means that no part of the implementation actually is
inherently tied to Listener-specific member functions any more.  With regard to
your suggestion, that is probably not much more than a philosophical
consideration.

However, I find the idea of a macro that silently analyses "this" in order to
implicitly create a type in case GET_LISTENER is called from a member function
actually worse than having it given explicitly.

It's like writing something like

  decltype(this) p;  // No guarantee this works

instead of

  Grob *p;

in a member function of Grob.

[dak, 2020-04-18 10:45:44]

On 2020/04/18 10:32:50, lemzwerg wrote:
> > I agree with the sentiment, but I fail to come up
> > with a naming choice that does not make the cure
> > worse than the problem.
> 
> I would simply use GET_LISTENERX (or GET_LISTENER1) for the longer call.

That's being obtuse.  Requires learning two things instead of one.

[dak, 2020-04-18 10:55:32]

On 2020/04/18 10:30:27, hanwenn wrote:
>
https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh
> File lily/include/listener.hh (left):
> 
>
https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener...
> lily/include/listener.hh:140: #define GET_LISTENER(cl, proc) get_listener
> (Callback_wrapper::make_smob<Listener::trampoline<cl, &cl::proc> > ())
> IMO, the problem is that the macro scopes 'proc' to come from 'cl', which is
> magical. Couldn't we change the calling convention to be 
> 
>   ctx->GET_LISTENER(&SomeContext::func)

Nope.  For one thing, you should be happy Smob_core no longer needs to provide a
member function just for the sake of Listener, making Listener less generic and
introducing a compile-time dependency.  For another, it requires the call site
to spell out the type of the expression ctx.  And the third is that the
implementation still needs to derive SomeContext from &SomeContext::func in
order to meet the requirements of the Callback_wrapper cell created.

> if we call this from within a SomeContext method itself, it could be 
> 
>    GET_LISTENER(&func)

No, it couldn't.  C++ member function pointer syntax cannot be done in that
manner.
 
> If so, would we need a macro at all?

The main purpose of the macro is not providing a wrapper for the Listener
creator, but rather for creating the template arguments for the Callback_wrapper
magic.  That one's the hardcore bit.  The Listener type just curries two SCM
values.

[dak, 2020-04-18 11:47:53]

Remove/move obviated includes of listener.hh

[Dan Eble, 2020-04-18 13:54:30]

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.c...
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...
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.

[dak, 2020-04-18 14:31:33]

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.c...
lily/global-context.cc:49: event_source ()->add_listener (GET_LISTENER
(static_cast<Context *> (this), create_context_from_event),
On 2020/04/18 13:54:30, Dan Eble wrote:
> This is a wart.

Yes and no.  The real wart is that &Global_context::create_context_from_event is
of type Context::* and not default-convertible to Global_context::* .  That is a
property of C++ that has triggered a number of painful consequences elsewhere in
our code base.

>  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> > ...

Exactly.

> 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.

I tried doing so.  It's complicated, and it only changes a single use case in
all of the code base.  Looked like more of a wart than this is.

> 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.

Probably a few hours of work, for a single use case.  Admittedly going to that
work might also provide a lead towards addressing the other instances where this
C++ problem has given us something to chew on.  I think that one thing that was
impacted was the standoff with Clang compatibility that Jonas finally addressed
by hard-coding a number of exceptions, in a similar vein.

> I'm sure you won't like complicating your macro,

At the current point of time, there is a single use case.

> 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.

I think that if we design a cure for the ::* problem, focusing it on this single
occurence seems like overkill.

I agree that reading it creates itchy fingers.

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...
lily/include/listener.hh:143: // This is somewhat more official, but way
overkill
On 2020/04/18 13:54:30, Dan Eble wrote:
> 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.

Overkill in loading a large header file of which only very little functionality
is required.  Now that listener.hh has been removed from a number of other
source files, the impact is more limited and the case for not using type_traits
is weaker.

One thing I have not mentioned speaking for the use of type_traits is that the
line

           (&decltype (dummy_deref (ptr))::proc)> > (),                \

would not get accepted as

           &decltype (dummy_deref (ptr))::proc> > (), \

by g++9 (parse error I think) suggesting that this ad-hoc implementation is
sailing uncomfortably close to triggering compiler bugs.  While this may also be
the case with the official header file, it's quite more probable that problems
would be detected and fixed without our intervention.

At any rate, you are pretty good at retracing the steps and considerations I
resolved on my own.

Ok, I'll let this be done by type_traits alone.  Personally, I consider it an
outrage that decltype (*(ptr)) apparently cannot be made to work on its own
here.  GNU's typeof probably could but it's not part of --std=c++11

If we consider type_traits as a sunk cost, I'll see whether I can find anything
in it to address the inheritance wart you complained of.

[Dan Eble, 2020-04-18 14:43:24]

On 2020/04/18 14:31:33, dak wrote:
> If we consider type_traits as a sunk cost, I'll see whether I can find
anything
> in it to address the inheritance wart you complained of.

As you already put in some time there, I wouldn't think less of you if you just
left the static_cast in Global_context and moved on.

[dak, 2020-04-18 17:33:32]

refactor a bit, use type_traits unconditionally

[dak, 2020-04-18 17:34:29]

Use C++ magic to derive proper member function pointer base class

[dak, 2020-04-18 17:53:34]

On 2020/04/18 14:43:24, Dan Eble wrote:
> On 2020/04/18 14:31:33, dak wrote:
> > If we consider type_traits as a sunk cost, I'll see whether I can find
> anything
> > in it to address the inheritance wart you complained of.
> 
> As you already put in some time there, I wouldn't think less of you if you
just
> left the static_cast in Global_context and moved on.

Well, after a refactoring (patch set 5) it was just a few lines (patch set 6)
but unfortunately this time definitely requiring some hand-spun type trait
punning.  Not particularly great.

[Dan Eble, 2020-04-18 18:16:05]

On 2020/04/18 17:53:34, dak wrote:
> Well, after a refactoring (patch set 5) it was just a few lines (patch set 6)
> but unfortunately this time definitely requiring some hand-spun type trait
> punning.  Not particularly great.

Not bad, either.  LGTM.

[dak, 2020-04-18 18:19:56]

Create and use type_traitor mfp_baseclass

[dak, 2020-04-18 19:49:10]

Cleanup.  Use std::remove_pointer rather than std:remove_reference.  Doh.

[dak, 2020-04-20 19:39:23]

Use the support from issue 5920 in callback.hh (included for reference but not
actually changed by this issue any more)