Re: Resolving standoffs

[David Kastrup]

Carl Sorensen <address@hidden> writes:

> On 4/13/20, 6:17 AM, "lilypond-devel on behalf of David Kastrup"
> <lilypond-devel-bounces+c_sorensen=address@hidden on behalf of
> address@hidden> wrote:
>
> It looks to me like we have only two people who have weighed in on the patch: 
>  the author (David K.) and one reviewer (Han-Wen).  Since we have only two 
> votes (1 for, 1 against), there is no resolution.
>
> I have been holding off comments, waiting to see where things go.  I'd
> now like to make my opinions known.

Let me correct some things here.  You may decide whether that makes a
difference to your thoughts.

> 1) David has an excellent track record of making challenging things easier to 
> do by thinking carefully about the implications of potentially arbitrary 
> syntax.  For example, the parser has been made more robust, and the scheme 
> interaction with the LilyPond parser has become much easier.  Because David 
> has such an excellent track record in this regard, I am inclined to favor 
> allowing this patch.
>
> 2) Han-Wen believes that this syntax change is potentially bad for at least 
> the following reasons:
>    a) The approach "seems laborious"
>    b)  The approach will make it hard to add new grob-properties at runtime 
> (David K. disagrees with this item -- in fact he believes it will force us to 
> provide a proper interface for registering user-added properties).
>    c)  Uses more memory -- lots of bytes per Grob.  David K. says it only 
> uses ~500 bytes per Grob type, not per Grob.
>    d) Interferes with Guile 2 adoption.  David K. says the current
> patch has no effect on Scheme; further work on Scheme is in the
> future.

All of those points are _not_ relevant for this patch.  They are
relevant for future work I intend to do that would be enabled by this
patch.  This patch, however, makes no assumptions whatsoever what kind
of followup code may be based on it.  It merely converts the use syntax
of the get_property and related macros from member call to explicit
call.  The details of any implementation using this are not topic of
this patch.

>    e) It's not necessary to make all these changes in the code base to
>    do the experiments that David K. wants to do.

It is necessary.  What isn't necessary is making them in the master
branch, but as the master branch diverges from any branch where
experiments are made, the basis of estimating performance gains goes
away unless one rebases.  And rebasing basically requires reapplying the
scripts and manual corrections.

> I think for me, the bottom line is that David has proposed a reason
> why the proposed new syntax is better than the existing syntax.  The
> reason is a little bit vague to me, but I trust David in this kind of
> situation.  Han-Wen has identified some potential warning issues about
> the new syntax, to which David has responded.  The only issue that
> Han-Wen raises for which I'm not satisfied an answer exists is item b)
> above.  If this patch breaks something users currently can do (adding
> new grob-properties at runtime) without providing another facility to
> do so, I think that's a legitimate concern.

It's totally not related to this patch.  It would be a legitimate
concern to be addressed in the followup work I sketched, and would want
addressing with providing dedicated interfaces and/or compatibility
code.

> On the other hand, I think that David's proposed memoization is likely
> to be very useful.
>
> David, do you agree that as it stands now, your patch precludes adding
> grob properties at runtime?

No.  This patch is syntactical in nature and involves neither a change
in performance nor in behavior.  I consider it quite unlikely that it
generates different code (apart from debug symbols of course).

> If so, would it be reasonable to add the infrastructure for adding
> grob properties at runtime to this patch?

It wouldn't belong in this patch (which does not affect any existing
functionality) but rather in the followup work.  Either as an
independent issue establishing a documented user interface, or as part
of the add-on patch, either likely with backward-compatibility code that
can be kept for a few versions.

> Han-Wen, do you agree that David has addressed your concerns about
> memory and Guile2?  If so, and he could resolve your concerns about
> adding grob properties at runtime, would you be willing to accept this
> syntax change?

The problem is that this patch does nothing whatsoever in this area.
This patch only allows for implementations taking into account the type
of property being changed but does not actually change the
implementation.

> From where I stand, the only think keeping me from giving a LGTM is
> the possible difficulty in adding new grob properties at runtime.  But
> I don’t have a clear sense of how significant that problem is.

It is irrelevant to this patch.  It is relevant to the followup work I
want to do, and I was not intending to break compatibility there.

-- 
David Kastrup