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/
