Re: Syntax: lyric_mode_music might be a MUSIC_FUNCTION (issue 343820043 by address@hidden)

[dak]

On 2018/04/25 12:22:18, knupero wrote:
> > I don't see the point in starting a formal review after this has
already been
> > discussed on the mailing list.

> I was hoping, apparently in vain, that the formal process would lead
to a more
> reasonable discussion.

You make it appear like "reasonable" means "with the desired outcome".

The formal process differs from the discussion not in who participates
but rather that it is connected with semi-automated procedures for
making a given patch progress into the code base, given that the
discussion on the trackers does not suggest the need for further
changes.

Proceeding to a formal patch suggestion when the discussion on the
developer list does not suggest the patch's design being in acceptable
state is just a waste of everyone's time.  This is different when we are
not talking about the design of the patch but rather its execution: in
that case the line-by-line review and commenting tools might be helpful
for getting the patch progress into a form which is acceptable.

The "formal" process is not a substitute for discussion, merely a tool
for allowing a more detailed review of the code.

> Apparently, however, this process also invites
> speculation about the amount of work someone is willing to invest, his
> qualifications and his honesty.

You are using hyperbole.

Your original patch suggestion was in
https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00097.html
.  This produces "not a note name" errors for things which are, well,
not a note name.  The error recovery of current grammar rules runs
through rules then actually producing lyric elements, and it would do so
regardless of whether the results are then used as parts of lyrics or
other music expressions.  Which is more a side effect of the current
grammar refactoring and an oversight in error processing than actually
useful behavior.

When I asked "how was the output correct?" without checking the output
of the current recovery path for strings encountered outside of lyrics
mode, you sent a reply
https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00108.html
.  This contains a whole lot of verbiage as well as PDFs.

At the end of the verbiage, there indeed is a sentence

> Attached is a 2nd version of the patch. With that extended version
_all_ examples above
>  compile without warnings/errors and give the desired result.

which I overlooked.  My reply
https://lists.gnu.org/archive/html/lilypond-devel/2018-04/msg00114.html
however comments on why there is a requirement of a braced music list
when switching to Lyrics mode.

Instead of replying to that objection, you formally propose the patch
which will not work reliably because of the reasons I pointed out.

While a careful rereading of the communication makes clear that I was
wrong stating:

> > In particular since this differs from your
> > originally proposed patch (where you did not switch to lyrics mode
at all) and
> > you disengenuously continued the discussion based on a modified
patch (the one
> > proposed here) which you then tacked onto your results without
further
> > mention.

> For the record: All the test results were obtained with a) an
unpatched lilypond
> and b) a lilypond with my originally proposed patch.

The record supports your statement here.

> This is easy to verify, and everybody is invited to do so. David: You
did not do
> that, apparently.

No, I didn't.  However, that the current error recovery path produces
lyrics events (regardless in which mode spurious strings appear) does
not imply that the flagged behavior would lead to sensible recognition
of lyrics input which differs from music input in a whole lot more
respects than just accepting words or not.  You can, for example, write
sentences with punctuation in lyrics mode, something which would not
work in the original code triggering errors but still producing lyrics
as recovery.

So there was no point in testing the patch as I knew perfectly well it
could not function comprehensively.  That led me to overlooking the
second patch which would fail in less obvious manners (without
triggering the error path) for input requiring lookahead, as spelled out
in the comment in the code and explained by myself in the discussion.

I apologize for misinterpreting your actions with regard to the second
patch I overlooked.  However, your disinterest in accepting any
explanation not in line with your desires and just going ahead ignoring
input in the expectation that this may somehow lead to more favorable
results is annoying, and I might have overreacted as a result of that
annoyance.

Whether it is inconvenient for your use case or not: mode-changing
commands can only accept a limited number of constructs, those known to
be delimited well enough that recognizing them does not require further
lookahead, or the next token will get scanned as lookahead in the wrong
mode, an action which cannot be redone.

It would be feasible to create a family of music function calls matching
that descriptions.  However, doing so would require a thorough
understanding of the current parser code and your manner of rejecting
input is likely going to make the acquisition of such knowledge
comparatively ineffective for you as well as reviewers.  That's not
necessarily a showstopper: after all, I acquired basically all of my
current parser chops at a time where its previous maintainers were
essentially no longer available.

However, it makes this particular project a bad idea for starting.

https://codereview.appspot.com/343820043/