Obviate method_finder methods (issue 551780043 by dak@gnu.org)

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

Obviate method_finder methods (issue 551780043 by dak@gnu.org)

Dev mailing list
Nice!  I don't really understand the C++ wizardry, but having to type
less macros in the normal code is certainly beneficial.

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
Reviewers: lemzwerg,

Message:
On 2020/04/19 05:53:34, lemzwerg wrote:
> Nice!  I don't really understand the C++ wizardry, but having to type
less
> macros in the normal code is certainly beneficial.

The wizardry is comparatively confined and mainly consists of a more
polished version of the mfp_baseclass thing that I needed to address
Dan's complaints about issue 5916 (which will need to get rebased on
this more elaborate version).  The advantage, of course, is that the
whole problem underlying issue 5600 that kept Clang from working (and
likely would have stopped GCC at some point of time) before Jonas found
a solution: gone.

The fundamental problem is that something like
&Global_context::create_context_from_event (a member function pointer to
an inherited function) is not of type void (Global_context::*)(SCM) but
of type void (Context::*)(SCM), and in the context of template argument
matching, you need to match the type exactly.

The basic C++ wizardry used here would already have worked in C++98 but
would have required a number of more specialised versions of
mfp_baseclass (for lack of template parameter packs).  I just failed to
come up with it before Dan asked for wart removal in issue 5916.

The disadvantage, of course is "I don't really understand the C++
wizardry".  C++11 has appropriated Perl's capacity of reading like line
noise, but in contrast to Perl, you never really get to the stage where
this line noise stops causing headaches.

At least this one saves a lot of other headaches.  And mfp_baseclass
does a well-defined job in a manner and with an interface typical to
C++11.  And it's few lines of headache in a single place rather than
something spread over all of the source code.

Description:
Obviate method_finder methods

The inheritance of various Translator::method_finder functions was a
comparatively fragile mess.  A helper class mfp_baseclass is able to
figure out the proper baseclass for a given member function pointer.
That allows removing the method_finder functions from the various
Translator-derived classes.

Please review this at https://codereview.appspot.com/551780043/

Affected files (+52, -61 lines):
  M lily/auto-beam-engraver.cc
  M lily/beam-engraver.cc
  M lily/context.cc
  M lily/include/callback.hh
  M lily/include/coherent-ligature-engraver.hh
  M lily/include/engraver.hh
  M lily/include/gregorian-ligature-engraver.hh
  M lily/include/ligature-engraver.hh
  M lily/include/translator.hh
  M lily/include/translator.icc
  M lily/kievan-ligature-engraver.cc
  M lily/mensural-ligature-engraver.cc
  M lily/phrasing-slur-engraver.cc
  M lily/repeat-tie-engraver.cc
  M lily/score-engraver.cc
  M lily/score-performer.cc
  M lily/translator-group.cc
  M lily/vaticana-ligature-engraver.cc



Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

jonas.hahnfeld
In reply to this post by Dev mailing list
A huge THANK YOU for this cleanup and getting rid of my
'TRANSLATOR_INHERIT' workaround. One request for using C++ style inline.

On the topic of 'C++ wizardry': While not completely straight-forward I
think mfp_baseclass is small enough to get your head around it if
needed. I mean half of it is remove_pointer which is part of type_traits
and one of the prime examples when starting with template programming.
The rest is hard to come up with (kudos that you did!), but that's what
you get for hiding static members in derived classes.


https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
File lily/include/callback.hh (right):

https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
lily/include/callback.hh:166: typedef typename
remove_pointer<decltype(strip_mfp (static_cast<T> (nullptr)))>::type
type;
I'd consider 'using type = ' instead which is more C++ style

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list

https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
File lily/include/callback.hh (right):

https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
lily/include/callback.hh:166: typedef typename
remove_pointer<decltype(strip_mfp (static_cast<T> (nullptr)))>::type
type;
On 2020/04/19 09:22:44, hahnjo wrote:
> I'd consider 'using type = ' instead which is more C++ style

I think you overestimate my C++ fu.  Can you spell this out?

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

jonas.hahnfeld
In reply to this post by Dev mailing list
On 2020/04/19 09:39:00, dak wrote:
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
> File lily/include/callback.hh (right):
>
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
> lily/include/callback.hh:166: typedef typename
remove_pointer<decltype(strip_mfp
> (static_cast<T> (nullptr)))>::type type;
> On 2020/04/19 09:22:44, hahnjo wrote:
> > I'd consider 'using type = ' instead which is more C++ style
>
> I think you overestimate my C++ fu.  Can you spell this out?

using type = typename remove_pointer<decltype(strip_mfp (static_cast<T>
(nullptr)))>::type;
to declare the type alias:
https://en.cppreference.com/w/cpp/language/type_alias

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
On 2020/04/19 09:55:03, hahnjo wrote:
> On 2020/04/19 09:39:00, dak wrote:
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
> > File lily/include/callback.hh (right):
> >
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
> > lily/include/callback.hh:166: typedef typename
> remove_pointer<decltype(strip_mfp
> > (static_cast<T> (nullptr)))>::type type;
> > On 2020/04/19 09:22:44, hahnjo wrote:
> > > I'd consider 'using type = ' instead which is more C++ style
> >
> > I think you overestimate my C++ fu.  Can you spell this out?
>
> using type = typename remove_pointer<decltype(strip_mfp
(static_cast<T>
> (nullptr)))>::type;
> to declare the type alias:
https://en.cppreference.com/w/cpp/language/type_alias

I see.  Basically squeezing in another piece of syntax where it has no
sensible place in order to have a variant of typedef that also works for
template specialisation of types.  Or in other words, more C++ style.  I
have the vague impression that the ability to do template specialisation
on typedefs would also be able to solve part of the job here in a more
elegant manner but right now my brain objects to more C++ style.  So
I'll confine myself to the change you suggest for now.  Will take
probably half the day to get to the next patch as I am also doing some
other reorganisation.

Thanks!

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

jonas.hahnfeld
In reply to this post by Dev mailing list
On 2020/04/19 10:55:52, dak wrote:
> On 2020/04/19 09:55:03, hahnjo wrote:
> > On 2020/04/19 09:39:00, dak wrote:
> > >
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
> > > File lily/include/callback.hh (right):
> > >
> > >
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
> > > lily/include/callback.hh:166: typedef typename
> > remove_pointer<decltype(strip_mfp
> > > (static_cast<T> (nullptr)))>::type type;
> > > On 2020/04/19 09:22:44, hahnjo wrote:
> > > > I'd consider 'using type = ' instead which is more C++ style
> > >
> > > I think you overestimate my C++ fu.  Can you spell this out?
> >
> > using type = typename remove_pointer<decltype(strip_mfp
(static_cast<T>
> > (nullptr)))>::type;
> > to declare the type alias:
> https://en.cppreference.com/w/cpp/language/type_alias
>
> I see.  Basically squeezing in another piece of syntax where it has no
sensible
> place in order to have a variant of typedef that also works for
template
> specialisation of types.  Or in other words, more C++ style.  I have
the vague
> impression that the ability to do template specialisation on typedefs
would also
> be able to solve part of the job here in a more elegant manner but
right now my
> brain objects to more C++ style.

Yes, I had the same feeling. However I think that the current solution
works because the template arguments are deduced from the function
argument. I don't currently see a way to make this work with template <>
using type =, but that would certainly be nice (and remove the need of
using decltype and remove_pointer).

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
On 2020/04/19 11:03:35, hahnjo wrote:
> On 2020/04/19 10:55:52, dak wrote:
> I have the vague
> > impression that the ability to do template specialisation on
typedefs would
> also
> > be able to solve part of the job here in a more elegant manner but
right now
> my
> > brain objects to more C++ style.
>
> Yes, I had the same feeling. However I think that the current solution
works
> because the template arguments are deduced from the function argument.
I don't
> currently see a way to make this work with template <> using type =,
but that
> would certainly be nice (and remove the need of using decltype and
> remove_pointer).

The principal problem is that the trampoline instantiation requires the
member function pointer as a non-type template parameter, and the type
of the non-type template parameter is not known in general.  So this
becomes a hen-and-egg problem.  Template specialisation could possibly
weasel around it but I am not sure about it.  It certainly is suspicious
that the otherwise rather comprehensive <type_traits> relying on such
techniques has nothing in its toolbox for that task.

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

nine.fierce.ballads
In reply to this post by Dev mailing list
On 2020/04/20 19:36:39, dak wrote:
> Polish and extend tools in callback.hh

LGTM.

I have a hunch that the MFPn_WRAP macros could be turned into something
more typical of C++, but I don't blame you for resting at this point.


https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
On 2020/04/20 23:26:07, Dan Eble wrote:
> On 2020/04/20 19:36:39, dak wrote:
> > Polish and extend tools in callback.hh
>
> LGTM.
>
> I have a hunch that the MFPn_WRAP macros could be turned into
something more
> typical of C++, but I don't blame you for resting at this point.

Not without a lot of effort since MFP1_WRAP and MFP2_WRAP rely on
"trampoline" in the current lexical scope doing something sensible.
They are unclean macros.  So one would have to overhaul the whole
trampoline concept to collect all trampolines in one scope (either :: or
Callback?::) and scatter specialisations over the files that want them.
A lot of work for what does not really look like a whole lot of gain.

https://codereview.appspot.com/551780043/

Reply | Threaded
Open this post in threaded view
|

Re: Obviate method_finder methods (issue 551780043 by dak@gnu.org)

David Kastrup
In reply to this post by Dev mailing list
On 2020/04/21 00:43:02, dak wrote:
> On 2020/04/20 23:26:07, Dan Eble wrote:
> > On 2020/04/20 19:36:39, dak wrote:
> > > Polish and extend tools in callback.hh
> >
> > LGTM.
> >
> > I have a hunch that the MFPn_WRAP macros could be turned into
something more
> > typical of C++, but I don't blame you for resting at this point.
>
> Not without a lot of effort since MFP1_WRAP and MFP2_WRAP rely on
"trampoline"
> in the current lexical scope doing something sensible.  They are
unclean macros.
>  So one would have to overhaul the whole trampoline concept to collect
all
> trampolines in one scope (either :: or Callback?::) and scatter
specialisations
> over the files that want them.  A lot of work for what does not really
look like
> a whole lot of gain.

Also if you were thinking of turning MFP0_WRAP into a _function_ taking
a pointer to member function argument, that will not work since the
whole trampoline creation concept relies on the particular pointer to
member being a template argument and thus fixed.  There is a separate
trampoline generated for each member function.

https://codereview.appspot.com/551780043/