Re: Just some short feedback

[David Kastrup]

Paul Morris <address@hidden> writes:

>> On Apr 27, 2015, at 6:20 PM, Trevor Daniels <address@hidden> wrote:
>> 
>> I'd like to second everything that Carl writes below,
>> and add my thanks to you, David.
>
> Let me add my +1 as well.  Thank you David for all your good work and
> for the update.  Here’s to more maintainable code and first-class
> scheme engravers!

Well, currently I am just going through the "more maintainable code"
phase.  Reorganizing the Listener stuff (and leaving a lot related
material alone for now) shows the statistics

address@hidden:/usr/local/tmp/lilypond$ git diff -w --stat origin issue4357
 lily/context.cc                         |  20 ++--
 lily/dispatcher-scheme.cc               |  13 ++-
 lily/dispatcher.cc                      |  26 +++--
 lily/engraver-group.cc                  |  10 +-
 lily/global-context.cc                  |   5 +-
 lily/grace-engraver.cc                  |   7 +-
 lily/include/context.hh                 |  12 +--
 lily/include/dispatcher.hh              |   5 +-
 lily/include/engraver-group.hh          |   4 +-
 lily/include/global-context.hh          |   2 +-
 lily/include/listener.hh                | 158 ++++++++++++++++++++-----------
 lily/include/scheme-listener.hh         |  41 --------
 lily/include/score-engraver.hh          |   6 +-
 lily/include/score-performer.hh         |   6 +-
 lily/include/smobs.hh                   |  19 +++-
 lily/include/translator-group.hh        |   2 +-
 lily/include/translator.hh              |   2 +-
 lily/include/translator.icc             |   3 +-
 lily/listener.cc                        |  41 --------
 lily/lyric-combine-music-iterator.cc    |  12 +--
 lily/midi-control-function-performer.cc |   8 +-
 lily/part-combine-iterator.cc           |   7 +-
 lily/scheme-engraver.cc                 |  33 +------
 lily/scheme-listener-scheme.cc          |  34 -------
 lily/scheme-listener.cc                 |  54 -----------
 lily/score-engraver.cc                  |  15 ++-
 lily/score-performer.cc                 |  15 ++-
 lily/smobs.cc                           |   7 ++
 lily/translator-group.cc                |   5 +-
 scm/part-combiner.scm                   |  14 ++-
 scm/scheme-engravers.scm                |   6 ++
 31 files changed, 222 insertions(+), 370 deletions(-)

And I might add that the number of comment lines has probably increased
significantly.  Of the macros GET_LISTENER, DECLARE_LISTENER,
IMPLEMENT_LISTENER only GET_LISTENER remains and it's straightforward to
use (and a one-liner).

All of the following ad_hoc macro stuff is just gone.  And there is no
mucking about with void * anymore: the stuff is type-safe.

-#define IMPLEMENT_LISTENER(cl, method)                  \
-void                                                    \
-cl :: method ## _callback (void *self, SCM ev)          \
-{                                                       \
-  cl *s = (cl *)self;                                   \
-  s->method (ev);                                       \
-}                                                       \
-void                                                    \
-cl :: method ## _mark (void *self)                      \
-{                                                       \
-  cl *s = (cl *)self;                                   \
-  scm_gc_mark (s->self_scm ());                         \
-}                                                       \
-bool                                                    \
-cl :: method ## _is_equal (void *a, void *b)            \
-{                                                       \
-  return a == b;                                        \
-}                                                       \
-Listener                                                \
-cl :: method ## _listener () const                      \
-{                                                       \
-  static Listener_function_table callbacks;             \
-  callbacks.listen_callback = &cl::method ## _callback; \
-  callbacks.mark_callback = &cl::method ## _mark;       \
-  callbacks.equal_callback = &cl::method ## _is_equal;  \
-  return Listener (this, &callbacks);                   \

A Listener was created from some this-pointer cast to void* and a
pointer to a Listener_callback table.  Set up fresh every time.

Now a listener is created from a function taking two Scheme arguments,
the first being a function with two arguments, and the second being the
first argument to use for calling this function.  And the resulting
Listener is employed as a Scheme function with a single argument.

There is a comment

+// A listener is essentially any procedure accepting a single argument
+// (namely an event).  The class Listener (or rather a smobbed_copy of
+// it) behaves like such a procedure and is composed of a generic
+// callback function accepting two arguments, namely a "target"
+// (usually an engraver instance) and the event.  Its Scheme
+// equivalent would be
+//
+// (define (make-listener callback target)
+//   (lambda (event) (callback target event)))
+//
+// The class construction is lightweight: as a Simple_smob, this is
+// only converted into Scheme when a smobbed_copy is created.

So a lot of black magic has turned into a straightforward composition
that could, efficiency aside, be equally well done in Scheme.

Then there is something new, namely
+// A callback wrapper creates a Scheme-callable version of a
+// non-static class member function callback which you can call with a
+// class instance and an event.
+//
+// If you have a class member function
+// void T::my_listen (SCM ev)
+// then Callback_wrapper::make_smob<&T::my_listen> ()
+// will return an SCM function roughly defined as
+// (lambda (target ev) (target->my_listen ev))

So a basic C++ member function is mapped into a straightforward
two-argument Scheme function (and can be replaced by one when desired,
something that I'll obviously be doing with the Scheme engraver next).

So the whole GET_LISTENER is now
+#define GET_LISTENER(cl, proc) get_listener (Callback_wrapper::make_smob<cl, 
&cl::proc> ())

where get_listener is a member function (of basically anything,
Translator, Context or whatever else) that ties a two-argument Scheme
function returned by Callback_wrapper::make_smob together with the
containing instance for the member function into a Listener, basically a
one-argument Scheme function.

You can basically crack the stuff open anywhere and substitute Scheme
(and it's going to be fun to do essentially that in order to deflate the
Scheme engraver implementation and throw the whole map-to-C++ layers
out).  And there is a lot more black magic that can be punctured and
drained from the current code in other places.

No, stuff does not become trivial in that manner.  But it stops being
totally opaque.

Well, and "first-class" Scheme engravers are really only marginally
related.  But I need to get the complexity under control in order to get
into a state where the implementation works well enough.

-- 
David Kastrup