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/
