Use Scheme_hash_table for keyword handling

Use Scheme_hash_table for keyword handling

[dak, 2020-04-27 12:22:34]

Minor consistency change

[lemzwerg, 2020-04-27 16:25:03]

LGTM

[hanwenn, 2020-04-27 21:22:59]

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

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:97: Protected_scm Lily_lexer::keytable_;
could move out of Lily_lexer.

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:102: if (!keytable_.is_bound ())
move this to a global init function? There is no reason to this in the
constructor.

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:202: Lily_lexer::keyword_list () const
Can you drop "ly:lexer-keywords" and delete this function?

[dak, 2020-04-27 22:07:07]

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

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:97: Protected_scm Lily_lexer::keytable_;
On 2020/04/27 21:22:59, hanwenn wrote:
> could move out of Lily_lexer.

It's only used in there and changing it elsewhere would be really bad, so why?

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
lily/lily-lexer.cc:102: if (!keytable_.is_bound ())
On 2020/04/27 21:22:59, hanwenn wrote:
> move this to a global init function? There is no reason to this in the
> constructor.

It has to be done when the Scheme system is up and before the first call of a
lexer.  I'll take a look at moving it outside; skeptical it will be a win in
readability/trustworthiness.

In order to keep access to keytable_ private, the initialization will need to
get done by a static member function.

[Dan Eble, 2020-04-27 22:31:33]

On 2020/04/27 22:07:07, dak wrote:
> https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc
> File lily/lily-lexer.cc (right):
> 
>
https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#ne...
> lily/lily-lexer.cc:97: Protected_scm Lily_lexer::keytable_;
> On 2020/04/27 21:22:59, hanwenn wrote:
> > could move out of Lily_lexer.
> 
> It's only used in there and changing it elsewhere would be really bad, so why?

I was about to say this.  And as it stands, it is obviously initialized before
it is used.  As a global, who knows?  I wouldn't spend any time trying to find
another way.

[dak, 2020-04-27 22:57:06]

Move keytable init out of constructor.  Frankly, I don't consider this an
improvement.

[dak, 2020-04-27 23:07:17]

On 2020/04/27 22:57:06, dak wrote:
> Move keytable init out of constructor.  Frankly, I don't consider this an
> improvement.

As anecdotal data, it took me about as long to get this working as the whole
other patch, and I had it segfault about half a dozen times and had another
half-dozen compiler errors while getting stuff right.

And doc_hash_table, all_ifaces and option_hash are initialised in the same
manner as what I did in the previous patch version.  And it's not like lexer
startup time is really going to factor in here a lot...  The previous code
generated an individual key table for every single lexer (except for cloned
lexers).

So without further feedback, I strongly lean towards pushing the previous
version.

[dak, 2020-04-27 23:17:30]

On 2020/04/27 23:07:17, dak wrote:
> On 2020/04/27 22:57:06, dak wrote:
> > Move keytable init out of constructor.  Frankly, I don't consider this an
> > improvement.
> 
> As anecdotal data, it took me about as long to get this working as the whole
> other patch, and I had it segfault about half a dozen times and had another
> half-dozen compiler errors while getting stuff right.
> 
> And doc_hash_table, all_ifaces and option_hash are initialised in the same
> manner as what I did in the previous patch version.  And it's not like lexer
> startup time is really going to factor in here a lot...  The previous code
> generated an individual key table for every single lexer (except for cloned
> lexers).
> 
> So without further feedback, I strongly lean towards pushing the previous
> version.

Though I'd have no problems pulling out the initialisation into a _private_
static member function (can be called from the constructor obviously) if people
consider it desirable to factor out that part of the initialisation.  Does not
need to access keytable_ but can just return the finished table.

[Dan Eble, 2020-04-27 23:24:19]

I prefer patch set 2.

https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll
File lily/lexer.ll (right):

https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll#newcode928
lily/lexer.ll:928: // use more SCM for this.
Is this what you've just done, or is there still more?

[dak, 2020-04-27 23:31:07]

This is a version I find ok.  Initialisation factored out and moved after
smobify_self (); call

[dak, 2020-04-27 23:37:35]

https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll
File lily/lexer.ll (right):

https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll#newcode928
lily/lexer.ll:928: // use more SCM for this.
On 2020/04/27 23:24:19, Dan Eble wrote:
> Is this what you've just done, or is there still more?

Will remove.  Not uploading separate patch just for that, though.

[dak, 2020-04-27 23:50:45]

Actually, now that make_keytable does not access any Lily_lexer internals, it
might as well be at file level like its data.

[hahnjo, 2020-05-01 10:35:51]

See also discussion at https://codereview.appspot.com/549920043