Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

Dev mailing list
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

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

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

Han-Wen Nienhuys-3
In reply to this post by Dev mailing list

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.hh#oldcode140
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?

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

Dev mailing list
In reply to this post by Dev mailing list
> 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.



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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
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.hh#oldcode140
> 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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

nine.fierce.ballads
In reply to this post by Dev mailing list
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/

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list

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/

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

nine.fierce.ballads
In reply to this post by Dev mailing list
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by dak@gnu.org)

nine.fierce.ballads
In reply to this post by Dev mailing list
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.


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