Stop \lyricmode { \skip 1.*3 } from failing.

Stop \lyricmode { \skip 1.*3 } from failing.

The problem was that the lexer preferred to match ".*" as a word since
the matched pattern was longer than the explicit pattern matching ".".
The word pattern is now amended so that it does not compete with the
single-character patterns.

[Vik Reykja, 2013-09-22 22:30:51]

I think a patch like this requires at least one regression test.

[dak, 2013-09-23 06:50:56]

On 2013/09/22 22:30:51, Vik Reykja wrote:
> I think a patch like this requires at least one regression test.

Well, actually the original lexer.ll patch would more likely have called for a
regression test as it covers a lot more change.

Would you want to propose one?  A sole \lyricmode { \skip 1.*3 } seems a bit
flimsy.

[Vik Reykja, 2013-09-23 16:16:45]

On Mon, Sep 23, 2013 at 8:50 AM, <dak@gnu.org> wrote:

> On 2013/09/22 22:30:51, Vik Reykja wrote:
>
>> I think a patch like this requires at least one regression test.
>>
>
> Well, actually the original lexer.ll patch would more likely have called
> for a regression test as it covers a lot more change.
>

Well sure, but that ship sailed a year ago.


> Would you want to propose one?  A sole \lyricmode { \skip 1.*3 } seems a
> bit flimsy.
>

I'm still very much a lilypond newcomer, and I don't have much knowledge
about lexers, so I don't really know what else that refactoring could have
affected.  I found one construct that it broke and I don't think that
including a regtest in the fix so it doesn't happen again is flimsy.
-- 
Vik

[dak, 2013-09-23 16:59:09]

Vik Reykja <vikreykja@gmail.com> writes:

> On Mon, Sep 23, 2013 at 8:50 AM, <dak@gnu.org> wrote:
>
>> On 2013/09/22 22:30:51, Vik Reykja wrote:
>>
>>> I think a patch like this requires at least one regression test.
>>>
>>
>> Well, actually the original lexer.ll patch would more likely have called
>> for a regression test as it covers a lot more change.
>>
>
> Well sure, but that ship sailed a year ago.

We're still phoning ships about hull leaks that sailed almost a decade
ago.  One would have thought they'd never make it out of port, but
people seem to be used to the bilge pump.

>> Would you want to propose one?  A sole \lyricmode { \skip 1.*3 }
>> seems a bit flimsy.
>>
>
> I'm still very much a lilypond newcomer, and I don't have much
> knowledge about lexers, so I don't really know what else that
> refactoring could have affected.  I found one construct that it broke
> and I don't think that including a regtest in the fix so it doesn't
> happen again is flimsy.

Hm.

-- 
David Kastrup

[janek, 2013-09-24 07:44:44]

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll
File lily/lexer.ll (right):

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll#newcode588
lily/lexer.ll:588: [^|*.=$#{}\"\\ \t\n\r\f0-9][^$#{}\"\\ \t\n\r\f0-9]* {
Hmm. From my point of view, this deserves some comment (but i don't insist).

[dak, 2013-09-24 16:15:39]

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll
File lily/lexer.ll (right):

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll#newcode588
lily/lexer.ll:588: [^|*.=$#{}\"\\ \t\n\r\f0-9][^$#{}\"\\ \t\n\r\f0-9]* {
On 2013/09/24 07:44:44, janek wrote:
> Hmm. From my point of view, this deserves some comment (but i don't insist).

I have a problem thinking of a comment that would add any information that is
not immediately apparent from the pattern.

Suggestions?

[janek, 2013-09-24 22:29:56]

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll
File lily/lexer.ll (right):

https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll#newcode588
lily/lexer.ll:588: [^|*.=$#{}\"\\ \t\n\r\f0-9][^$#{}\"\\ \t\n\r\f0-9]* {
On 2013/09/24 16:15:39, dak wrote:
> On 2013/09/24 07:44:44, janek wrote:
> > Hmm. From my point of view, this deserves some comment (but i don't insist).
> 
> I have a problem thinking of a comment that would add any information that is
> not immediately apparent from the pattern.
> 
> Suggestions?

Maybe "because of regex greediness, we have to additionally exclude patterns
beginning with |*.= (without this \lyricmode { \skip 1.*3 } fails)"?

For me the regex itself required a few moments of thinking to understand.

Janek

[dak, 2013-09-24 23:16:31]

On 2013/09/24 22:29:56, janek wrote:
> https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll
> File lily/lexer.ll (right):
> 
> https://codereview.appspot.com/13256053/diff/1/lily/lexer.ll#newcode588
> lily/lexer.ll:588: [^|*.=$#{}\"\\ \t\n\r\f0-9][^$#{}\"\\ \t\n\r\f0-9]* {
> On 2013/09/24 16:15:39, dak wrote:
> > On 2013/09/24 07:44:44, janek wrote:
> > > Hmm. From my point of view, this deserves some comment (but i don't
insist).
> > 
> > I have a problem thinking of a comment that would add any information that
is
> > not immediately apparent from the pattern.
> > 
> > Suggestions?
> 
> Maybe "because of regex greediness, we have to additionally exclude patterns
> beginning with |*.= (without this \lyricmode { \skip 1.*3 } fails)"?

Well, multiple matching regexps are messy and might call for an appropriate
comment.  But the whole point of the change is not to have multiple matching
regexps any more.

The original commit message of the commit that failed to do the job was:

commit 38a4081efa4a8ee2f5da780ca0ed2991627afc46
Author: David Kastrup <dak@gnu.org>
Date:   Sun Sep 30 02:21:00 2012 +0200

    Issue 2869: Regularize lyrics lexer mode
    
    That makes lyrics mode rather similar to markup mode regarding how
    words are formed.  {} are never considered part of words unless
    enclosed in quotes.  Unquoted words do not contain whitespace, braces,
    quotes, backslashes, numbers or Scheme expressions.  In addition, they
    cannot start with * . = and | since that would mess with duration,
    assignment and barcheck syntax.  This removes some remaining
    TeX-oriented cruft in the lexer.  The set of word-non-starters might
    need revisiting, but at least the regtests seem to pass.

The text that is relevant here is "In addition, [unquoted words] cannot start
with * . = and | since that would mess with duration, assignment and barcheck
syntax."

After the fix, the pattern _explicitly_ (rather than implicitly by pattern order
which did not work) states that unquoted words cannot start with * . = and |. 
The pattern is now a literal rendition of the description.

> For me the regex itself required a few moments of thinking to understand.

The problem is that you want a comment describing the problems with
code/patterns that is no longer there.  I don't think that's helpful.  A comment
is called for when you use a trick to avoid a problem.  But in this case, the
problem was avoided by stopping to use a trick and instead being boringly
explicit in the pattern.

It's quite redundant now to state "this pattern won't match something starting
with * . = or |" since the pattern explicitly excludes those characters in its
first position.  My first attempt was to give the first single-char pattern
priority back by using
[*.=]/.*
(a "trailing context" which did not actually work because of technical
restrictions).  That would both have been fragile _and_ would have called for a
comment in order to explain its purpose.  But the purpose of a pattern
[^*.=| ...
is not to match * . = | in the first position of the pattern.  That's basic. 
Comments can't hope to explain everything that's basic. They should tell the
story that the code doesn't tell, at least not on its most direct surface level.
 And the code now tells the story in the most blunt way that is possible.  Of
course we can add a comment of the "we could do this in a trickier way that does
not actually work" kind, but that's not helpful at all.

It does not belong in the code.  It might belong in the issue tracker, perhaps
in the commit message.  Just as a record of what went wrong.  But the code, as
it is written, does not offer a temptation of changing it back to the buggy
previous version.  It was not more elegant or obvious or clearer.

[janek, 2013-09-26 10:51:34]

2013/9/25  <dak@gnu.org>:
>
>> > On 2013/09/24 07:44:44, janek wrote:
>> >> Hmm. From my point of view, this deserves some comment
>> >> (but i don't insist).
>
> Well, multiple matching regexps are messy and might call for an
> appropriate comment.  But the whole point of the change is not to have
> multiple matching regexps any more.
>
> The original commit message of the commit that failed to do the job was:
>
> commit 38a4081efa4a8ee2f5da780ca0ed2991627afc46
> Author: David Kastrup <dak@gnu.org>
> Date:   Sun Sep 30 02:21:00 2012 +0200
>
>     Issue 2869: Regularize lyrics lexer mode
>
>     That makes lyrics mode rather similar to markup mode regarding how
>     words are formed.  {} are never considered part of words unless
>     enclosed in quotes.  Unquoted words do not contain whitespace,
> braces,
>     quotes, backslashes, numbers or Scheme expressions.  In addition,
> they
>     cannot start with * . = and | since that would mess with duration,
>     assignment and barcheck syntax.  This removes some remaining
>     TeX-oriented cruft in the lexer.  The set of word-non-starters might
>     need revisiting, but at least the regtests seem to pass.
>
> The text that is relevant here is "In addition, [unquoted words] cannot
> start with * . = and | since that would mess with duration, assignment
> and barcheck syntax."
>
> After the fix, the pattern _explicitly_ (rather than implicitly by
> pattern order which did not work) states that unquoted words cannot
> start with * . = and |.  The pattern is now a literal rendition of the
> description.
>
>
>> For me the regex itself required a few moments of thinking to
>> understand.
>
> The problem is that you want a comment describing the problems with
> code/patterns that is no longer there.  I don't think that's helpful.  A
> comment is called for when you use a trick to avoid a problem.  But in
> this case, the problem was avoided by stopping to use a trick and
> instead being boringly explicit in the pattern.
>
> It's quite redundant now to state "this pattern won't match something
> starting with * . = or |" since the pattern explicitly excludes those
> characters in its first position.  My first attempt was to give the
> first single-char pattern priority back by using
> [*.=]/.*
> (a "trailing context" which did not actually work because of technical
> restrictions).  That would both have been fragile _and_ would have
> called for a comment in order to explain its purpose.  But the purpose
> of a pattern
> [^*.=| ...
> is not to match * . = | in the first position of the pattern.  That's
> basic.  Comments can't hope to explain everything that's basic. They
> should tell the story that the code doesn't tell, at least not on its
> most direct surface level.  And the code now tells the story in the most
> blunt way that is possible.  Of course we can add a comment of the "we
> could do this in a trickier way that does not actually work" kind, but
> that's not helpful at all.
>
> It does not belong in the code.  It might belong in the issue tracker,
> perhaps in the commit message.  Just as a record of what went wrong.
> But the code, as it is written, does not offer a temptation of changing
> it back to the buggy previous version.  It was not more elegant or
> obvious or clearer.

Uh, David, thanks for this explanation, but i'm afraid it was not
needed.  If the code doesn't need a comment, just say so.

Now, speaking in general: i don't understand that when Mike submits a
patch, you often complain that it's not commented well enough, but
when I ask you for comments, you usually say "this doesn't need any
comment".

Apparently both you and Mike consider what you write to be self-explanatory.

Maybe you are right that for an experienced developer Mike's code
isn't self-explanatory, and your code is.  All i know is that for me
neither is self-explanatory.  Of course, i'm not an experienced
programmer.  But then, i've been working on LilyPond since 3 years
now, and that seems a pretty long time.

I am sorry that (apparently) you don't care whether i can understand
your patches easily or not.  Also, from my perspective, it seems that
you consider yourself to be always right:
"i think this deserves a regtest" "no, it doesn't"
"i think this deserves a comment" "no, it doesn't"

I suppose that you're not doing this intentionally.  But it drives me
crazy anyway.

Anyway, it seems that when i try to review your patches the first and
foremost result is that we both loose time, so i suppose that i should
stop reviewing your patches :-(

Janek

[dak, 2013-09-26 12:12:12]

On 2013/09/26 10:51:34, janek wrote:
>
> > It does not belong in the code.  It might belong in the issue tracker,
> > perhaps in the commit message.  Just as a record of what went wrong.
> > But the code, as it is written, does not offer a temptation of changing
> > it back to the buggy previous version.  It was not more elegant or
> > obvious or clearer.
> 
> Uh, David, thanks for this explanation, but i'm afraid it was not
> needed.  If the code doesn't need a comment, just say so.
> 
> Now, speaking in general: i don't understand that when Mike submits a
> patch, you often complain that it's not commented well enough, but
> when I ask you for comments, you usually say "this doesn't need any
> comment".
> 
> Apparently both you and Mike consider what you write to be
> self-explanatory.

git blame lily/lexer.ll|grep "//\|/\*"|wc -l
146

git blame lily/lexer.ll|grep "//\|/\*"|grep Kastrup|wc -l
120

git blame lily/lexer.ll|wc -l
1382

git blame lily/lexer.ll|grep Kastrup|wc -l
677

Are you sure that your characterization is fair?  I'm responsible for
less than half the lines in the lexer, but for more than 80% of the
lines starting a comment in the lexer.

It's a bit hard to do the same kind of check on a typical C++ file
since superficially most lines are blamed on whoever ran the indenting
tool last.  lily/lexer.ll is not automatically indented, so it's
somewhat more reliable.

When my typical complaint with a commit from Mike focuses on the
_number_ of comments, it's usually because there are about 10 lines of
comments in 5000 lines of code (which seems indeed out of proportion
based purely on the numbers, particularly when the whole _framework_
put together in those 5000 lines is non-trivial, opaque and mostly
non-discoverable), and half of the comments are wrong.

Now statistics can definitely be misleading as indeed a comment can be
worthless and/or distracting and always is in danger of running out of
sync with the code.  So it needs to have value _separate_ from the
code in order to be worth it.  If it repeats the story spelled out in
the code (and this is what you ask for here), it's useless.  If it
_summarizes_ a passage of code, it's already separately useful.  If it
puts code into context, it's _quite_ useful.

> Maybe you are right that for an experienced developer Mike's code
> isn't self-explanatory, and your code is.  All i know is that for me
> neither is self-explanatory.

So you say that a pattern written starting with

[^.*|...

does not tell you that it is supposed to match something not starting
with a . * | and whatever else is listed here.  That's a matter of not
knowing Flex and/or regular expressions, not a matter of not being
able to follow the logic of the code.

That's like asking for a comment like

    x += 2;  // increase x by 2

which is not helpful.

> Of course, i'm not an experienced
> programmer.  But then, i've been working on LilyPond since 3 years
> now, and that seems a pretty long time.
> 
> I am sorry that (apparently) you don't care whether i can understand
> your patches easily or not.

You will understand a patch to the lexer _only_ if you know the
language the lexer is written in.  A code review that has to rely on
_comments_ in order to "understand" every facet of the code (rather
than larger contexts and/or its design) is basically useless as any
divergence between code and comment can't be detected.


> Also, from my perspective, it seems that
> you consider yourself to be always right:
> "i think this deserves a regtest" "no, it doesn't"

If you consider "no, it doesn't" as the same as "a regtest in that
area should at least cover that and that cases.  Would you like to
propose one?", sure.

Can you point out to any case where I flat out stated that a regtest
would not be warranted?

> "i think this deserves a comment" "no, it doesn't"

Can you please propose a comment that does not

a) talk about a state that's not actually in the code and won't get
   there accidentally
b) is not _immediately_ _literally_ obvious from the code for anybody
   knowing the language that something is written in?

Comments should increase the signal/noise ratio of a program, not
decrease it.  They need to tell the story that is _not_ explicitly
coded.

> I suppose that you're not doing this intentionally.  But it drives
> me crazy anyway.  Anyway, it seems that when i try to review your
> patches the first and foremost result is that we both loose time,

Does that mean that I can save myself the effort to _explain_ my
decisions since you would consider it a loss of time to try
understanding them?

You _are_ aware that it takes much less effort to put in a useless
comment rather than explain why it wouldn't help in a particular case?
So obviously my main aim is not to reduce the amount of work.

> so i suppose that i should stop reviewing your patches

Depends on your goals.

[janek, 2013-09-26 12:55:50]

2013/9/26  <dak@gnu.org>:
> On 2013/09/26 10:51:34, janek wrote:
>> Uh, David, thanks for this explanation, but i'm afraid it was not
>> needed.  If the code doesn't need a comment, just say so.
>>
>> Now, speaking in general: i don't understand that when Mike submits a
>> patch, you often complain that it's not commented well enough, but
>> when I ask you for comments, you usually say "this doesn't need any
>> comment".
>>
>> Apparently both you and Mike consider what you write to be
>> self-explanatory.
>
> git blame lily/lexer.ll|grep "//\|/\*"|wc -l
> 146
>
> git blame lily/lexer.ll|grep "//\|/\*"|grep Kastrup|wc -l
> 120
>
> git blame lily/lexer.ll|wc -l
> 1382
>
> git blame lily/lexer.ll|grep Kastrup|wc -l
> 677
>
> Are you sure that your characterization is fair?  I'm responsible for
> less than half the lines in the lexer, but for more than 80% of the
> lines starting a comment in the lexer.

That's very commendable, but i think you missed my point.  I didn't
say that you don't put comments in your code.  I said that _what you
write_ (i.e. code+comments that you write) is not self-explanatory for
me.  It's quite probable that i'm simply too dumb or unskilled to
understand things that an average programmer should understand.

> When my typical complaint with a commit from Mike focuses on the
> _number_ of comments, it's usually because there are about 10 lines of
> comments in 5000 lines of code (which seems indeed out of proportion
> based purely on the numbers, particularly when the whole _framework_
> put together in those 5000 lines is non-trivial, opaque and mostly
> non-discoverable), and half of the comments are wrong.
>
> Now statistics can definitely be misleading as indeed a comment can be
> worthless and/or distracting and always is in danger of running out of
> sync with the code.  So it needs to have value _separate_ from the
> code in order to be worth it.  If it repeats the story spelled out in
> the code (and this is what you ask for here),

Note that only first two lines of my reply was about this particular
patch.  Everything else was said about my general experience.  So, i'm
not insisting that there should be a comment here.

>> Maybe you are right that for an experienced developer Mike's code
>> isn't self-explanatory, and your code is.  All i know is that for me
>> neither is self-explanatory.
>
> So you say that a pattern written starting with
>
> [^.*|...
>
> does not tell you that it is supposed to match something not starting
> with a . * | and whatever else is listed here.  That's a matter of not
> knowing Flex and/or regular expressions, not a matter of not being
> able to follow the logic of the code.

Yes, you're right.

> That's like asking for a comment like
>
>     x += 2;  // increase x by 2
>
> which is not helpful.

yes, you're right.

>> Of course, i'm not an experienced
>> programmer.  But then, i've been working on LilyPond since 3 years
>> now, and that seems a pretty long time.
>>
>> I am sorry that (apparently) you don't care whether i can understand
>> your patches easily or not.
>
>
> You will understand a patch to the lexer _only_ if you know the
> language the lexer is written in.  A code review that has to rely on
> _comments_ in order to "understand" every facet of the code (rather
> than larger contexts and/or its design) is basically useless as any
> divergence between code and comment can't be detected.

yes, you're right.

>> Also, from my perspective, it seems that
>> you consider yourself to be always right:
>> "i think this deserves a regtest" "no, it doesn't"
>
>
> If you consider "no, it doesn't" as the same as "a regtest in that
> area should at least cover that and that cases.  Would you like to
> propose one?", sure.
>
> Can you point out to any case where I flat out stated that a regtest
> would not be warranted?

No, i don't have such example.

>> "i think this deserves a comment" "no, it doesn't"
>
> Can you please propose a comment that does not
>
> a) talk about a state that's not actually in the code and won't get
>    there accidentally
> b) is not _immediately_ _literally_ obvious from the code for anybody
>    knowing the language that something is written in?

No, i cannot.

> Comments should increase the signal/noise ratio of a program, not
> decrease it.  They need to tell the story that is _not_ explicitly
> coded.

yes, you're right.

>> I suppose that you're not doing this intentionally.  But it drives
>> me crazy anyway.  Anyway, it seems that when i try to review your
>> patches the first and foremost result is that we both loose time,
>
> Does that mean that I can save myself the effort to _explain_ my
> decisions since you would consider it a loss of time to try
> understanding them?

Not really.  I think that you can save yourself the effort to explain
your decisions - at least in this particular case - because that's an
ineffective use of your time.  Also, having me understand your
decisions is not required for anything.

In any case, i didn't ask for explanations in the first place.  I just
suggested adding a comment, without requiring you to explain yourself
if you didn't consider adding a comment appropriate.

> You _are_ aware that it takes much less effort to put in a useless
> comment rather than explain why it wouldn't help in a particular case?

Yes, i am.

>> so i suppose that i should stop reviewing your patches
>
> Depends on your goals.

My goal is to do something useful for lilypond.  I don't think this
discussion is useful for lilypond :-(