Use a hash table for the lexer keywords

Use a hash table for the lexer keywords

Get rid of unused function ly:lexer-keywords.

[lemzwerg, 2020-04-20 08:14:30]

LGTM

[hahnjo, 2020-04-20 08:17:06]

https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lex...
File lily/include/lily-lexer.hh (right):

https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lex...
lily/include/lily-lexer.hh:23: #include <unordered_map>
Please put C++ includes last. I know that Google advocates differently, but
having them first makes it very easy to rely on that in other header files.
Instead every header file should include what they use.

[hanwenn, 2020-04-20 08:33:53]

jonas

[hanwenn, 2020-04-20 08:34:06]

https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lex...
File lily/include/lily-lexer.hh (right):

https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lex...
lily/include/lily-lexer.hh:23: #include <unordered_map>
On 2020/04/20 08:17:05, hahnjo wrote:
> Please put C++ includes last. I know that Google advocates differently, but
> having them first makes it very easy to rely on that in other header files.
> Instead every header file should include what they use.

Done.

[hahnjo, 2020-04-20 08:50:08]

LGTM, nice cleanup.

[hanwenn, 2020-04-27 08:39:57]

drop table from lexer class

[hanwenn, 2020-04-27 08:41:31]

unused hdr

[dak, 2020-04-27 08:59:24]

Without having looked all that much at the code:

When the parser sees some \blabla it will generally first have to check for a
keyword and (when it has no match) afterwards for a variable with that name, and
a lot of those are actually music functions these days that used to be keyword.

Wouldn't it make sense to just convert \xxx into a symbol early on in the lexer?
 That is essentially a hash code, and we have lookups for those.  It seems
wasteful to use two completely different ways of hashing a string in succession
when we can just turn this into a symbol early on and work with that instead.

[hanwenn, 2020-04-27 08:59:46]

unused hdr

[hanwenn, 2020-04-27 09:00:20]

unused hdr

[hanwenn, 2020-04-27 09:06:20]

On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote:
>
> Without having looked all that much at the code:
>
> When the parser sees some \blabla it will generally first have to check
> for a keyword and (when it has no match) afterwards for a variable with
> that name, and a lot of those are actually music functions these days
> that used to be keyword.
>
> Wouldn't it make sense to just convert \xxx into a symbol early on in
> the lexer?  That is essentially a hash code, and we have lookups for
> those.  It seems wasteful to use two completely different ways of
> hashing a string in succession when we can just turn this into a symbol
> early on and work with that instead.
>
> https://codereview.appspot.com/549920043/



-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[hanwenn, 2020-04-27 09:06:43]

On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote:
> When the parser sees some \blabla it will generally first have to check
> for a keyword and (when it has no match) afterwards for a variable with
> that name, and a lot of those are actually music functions these days
> that used to be keyword.
>
> Wouldn't it make sense to just convert \xxx into a symbol early on in
> the lexer?  That is essentially a hash code, and we have lookups for
> those.  It seems wasteful to use two completely different ways of
> hashing a string in succession when we can just turn this into a symbol
> early on and work with that instead.

If you can make Bison work off SCM symbol values, be my guest.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[dak, 2020-04-27 09:11:16]

Han-Wen Nienhuys <hanwenn@gmail.com> writes:

> On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote:
>> When the parser sees some \blabla it will generally first have to check
>> for a keyword and (when it has no match) afterwards for a variable with
>> that name, and a lot of those are actually music functions these days
>> that used to be keyword.
>>
>> Wouldn't it make sense to just convert \xxx into a symbol early on in
>> the lexer?  That is essentially a hash code, and we have lookups for
>> those.  It seems wasteful to use two completely different ways of
>> hashing a string in succession when we can just turn this into a symbol
>> early on and work with that instead.
>
> If you can make Bison work off SCM symbol values, be my guest.

Yo may have missed the memo, but everything passed into and out of the
parser and Lexer these days is SCM.  And for looking up a token ID from
an SCM symbol, Guile has hashtables, and we also habe Scm_hashtable.

-- 
David Kastrup

[hahnjo, 2020-04-27 10:28:28]

As testing releaved, there's now
#include <unordered_map>
missing in lily-lexer.cc

https://codereview.appspot.com/549920043/diff/566010043/lily/lily-lexer.cc
File lily/lily-lexer.cc (right):

https://codereview.appspot.com/549920043/diff/566010043/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:40: class Keyword_ent
I think this class isn't used anymore

[dak, 2020-04-27 11:58:11]

On 2020/04/27 09:11:16, dak wrote:
> Han-Wen Nienhuys <mailto:hanwenn@gmail.com> writes:
> 
> > On Mon, Apr 27, 2020 at 10:59 AM <mailto:dak@gnu.org> wrote:
> >> When the parser sees some \blabla it will generally first have to check
> >> for a keyword and (when it has no match) afterwards for a variable with
> >> that name, and a lot of those are actually music functions these days
> >> that used to be keyword.
> >>
> >> Wouldn't it make sense to just convert \xxx into a symbol early on in
> >> the lexer?  That is essentially a hash code, and we have lookups for
> >> those.  It seems wasteful to use two completely different ways of
> >> hashing a string in succession when we can just turn this into a symbol
> >> early on and work with that instead.
> >
> > If you can make Bison work off SCM symbol values, be my guest.
> 
> Yo may have missed the memo, but everything passed into and out of the
> parser and Lexer these days is SCM.  And for looking up a token ID from
> an SCM symbol, Guile has hashtables, and we also habe Scm_hashtable.
> 
> -- 
> David Kastrup

Tracker issue: 5946 (https://sourceforge.net/p/testlilyissues/issues/5946/)
Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
Issue description:
  Use Scheme_hash_table for keyword handling

[hanwenn, 2020-04-27 20:37:32]

add missing header

[dak, 2020-04-28 10:37:24]

Regarding the "Get rid of unused function ly:lexer-keywords": that one was added
with
commit 1ae6f5710e714f13da1bfbfbd840835316618d1f
Author: Han-Wen Nienhuys <hanwen@xs4all.nl>
Date:   Thu Dec 14 14:10:56 2006 +0100

    add ly:lexer-keywords to make keyword extraction easier and exact.

but there appears to be no commit that ever used it.  The sentiment expressed in
the commit message appears reasonable, and with the alternative proposal in
issue 5946, the implementation comes basically free.  Do you remember what you
were planning to do with it at the time you committed it?

[hahnjo, 2020-04-30 07:42:16]

On 2020/04/27 11:58:11, dak wrote:
> Tracker issue: 5946 (https://sourceforge.net/p/testlilyissues/issues/5946/)
> Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
> Issue description:
>   Use Scheme_hash_table for keyword handling

We should probably decide between these two approaches, we likely can't do both.
I see the advantage of using SCM values for everything, but I'm not familiar
with the code.

[dak, 2020-04-30 07:58:09]

On 2020/04/30 07:42:16, hahnjo wrote:
> On 2020/04/27 11:58:11, dak wrote:
> > Tracker issue: 5946 (https://sourceforge.net/p/testlilyissues/issues/5946/)
> > Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
> > Issue description:
> >   Use Scheme_hash_table for keyword handling
> 
> We should probably decide between these two approaches, we likely can't do
both.
> I see the advantage of using SCM values for everything, but I'm not familiar
> with the code.

I think it makes more sense here not to introduce new data types for a job that
is inherent to Scheme's operation.  Having both patches on countdown at the same
time obviously does not make sense: if discussion is required (Han-Wen?), it
would make sense to stop both until this is resolved.

An independent component is the removal of ly:lexer-keywords .  There is no
indication that it ever has been used; it is cheap to provide with my version. 
However, revisiting its code I also see that it takes a lexer as an argument. 
My patch, like Han-Wen's, stops lexers from having their individual keytable (an
implementation detail that was never used for any purpose).  So even if the
function were retained, letting it take an argument, while making for backwards
compatibility, does not appear to make sense.  This could be addressed in a
separate patch/issue.  Or it could be removed in a separate patch/issue.

One possible use for it would be using LilyPond itself for generating syntax
highlighting for editors.  The current solutions rather extract stuff from the
source I seem to remember.

[hanwenn, 2020-05-01 11:15:29]

On 2020/04/30 07:58:09, dak wrote:
> On 2020/04/30 07:42:16, hahnjo wrote:
> > On 2020/04/27 11:58:11, dak wrote:
> > > Tracker issue: 5946
(https://sourceforge.net/p/testlilyissues/issues/5946/)
> > > Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
> > > Issue description:
> > >   Use Scheme_hash_table for keyword handling
> > 
> > We should probably decide between these two approaches, we likely can't do
> both.
> > I see the advantage of using SCM values for everything, but I'm not familiar
> > with the code.
> 
> I think it makes more sense here not to introduce new data types for a job
that
> is inherent to Scheme's operation.  Having both patches on countdown at the
same
> time obviously does not make sense: if discussion is required (Han-Wen?), it
> would make sense to stop both until this is resolved.
> 
> An independent component is the removal of ly:lexer-keywords .  There is no
> indication that it ever has been used; it is cheap to provide with my version.

> However, revisiting its code I also see that it takes a lexer as an argument. 
> My patch, like Han-Wen's, stops lexers from having their individual keytable
(an
> implementation detail that was never used for any purpose).  So even if the
> function were retained, letting it take an argument, while making for
backwards
> compatibility, does not appear to make sense.  This could be addressed in a
> separate patch/issue.  Or it could be removed in a separate patch/issue.
> 
> One possible use for it would be using LilyPond itself for generating syntax
> highlighting for editors.  The current solutions rather extract stuff from the
> source I seem to remember.

1) we can take dak's patch if you prefer. They're roughly equivalent anyway.

2) I suppose ly:lexer-keywords was for editor modes, but it hasn't been used for
that.
Updating the scripts to generate the modes is extra work, so I vote for removing
it. If 
nobody bothered in the last 13 years, it can't be that important.

[dak, 2020-05-01 11:19:28]

On 2020/05/01 11:15:29, hanwenn wrote:
> On 2020/04/30 07:58:09, dak wrote:
> > On 2020/04/30 07:42:16, hahnjo wrote:
> > > On 2020/04/27 11:58:11, dak wrote:
> > > > Tracker issue: 5946
> (https://sourceforge.net/p/testlilyissues/issues/5946/)
> > > > Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
> > > > Issue description:
> > > >   Use Scheme_hash_table for keyword handling
> > > 
> > > We should probably decide between these two approaches, we likely can't do
> > both.
> > > I see the advantage of using SCM values for everything, but I'm not
familiar
> > > with the code.
> > 
> > I think it makes more sense here not to introduce new data types for a job
> that
> > is inherent to Scheme's operation.  Having both patches on countdown at the
> same
> > time obviously does not make sense: if discussion is required (Han-Wen?), it
> > would make sense to stop both until this is resolved.
> > 
> > An independent component is the removal of ly:lexer-keywords .  There is no
> > indication that it ever has been used; it is cheap to provide with my
version.
> 
> > However, revisiting its code I also see that it takes a lexer as an
argument. 
> > My patch, like Han-Wen's, stops lexers from having their individual keytable
> (an
> > implementation detail that was never used for any purpose).  So even if the
> > function were retained, letting it take an argument, while making for
> backwards
> > compatibility, does not appear to make sense.  This could be addressed in a
> > separate patch/issue.  Or it could be removed in a separate patch/issue.
> > 
> > One possible use for it would be using LilyPond itself for generating syntax
> > highlighting for editors.  The current solutions rather extract stuff from
the
> > source I seem to remember.
> 
> 1) we can take dak's patch if you prefer. They're roughly equivalent anyway.
> 
> 2) I suppose ly:lexer-keywords was for editor modes, but it hasn't been used
for
> that.
> Updating the scripts to generate the modes is extra work, so I vote for
removing
> it. If 
> nobody bothered in the last 13 years, it can't be that important.

I'll make the removal a separate issue (and obviously commit) once the "base
patch" is in.  That makes it possible to find, reference, and possibly revert
the removal in case that is ever wanted.

[hahnjo, 2020-05-01 11:22:12]

On 2020/05/01 11:19:28, dak wrote:
> On 2020/05/01 11:15:29, hanwenn wrote:
> > 1) we can take dak's patch if you prefer. They're roughly equivalent anyway.
> > 
> > 2) I suppose ly:lexer-keywords was for editor modes, but it hasn't been used
> for
> > that.
> > Updating the scripts to generate the modes is extra work, so I vote for
> removing
> > it. If 
> > nobody bothered in the last 13 years, it can't be that important.
> 
> I'll make the removal a separate issue (and obviously commit) once the "base
> patch" is in.  That makes it possible to find, reference, and possibly revert
> the removal in case that is ever wanted.

SGTM