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

[dak]

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),
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.hh#newcode143
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.

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