Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(75)

Issue 549920043: Use a hash table for the lexer keywords (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by hanwenn
Modified:
4 weeks, 1 day ago
Reviewers:
dak, lemzwerg, hahnjo
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Use a hash table for the lexer keywords Get rid of unused function ly:lexer-keywords.

Patch Set 1 #

Total comments: 2

Patch Set 2 : jonas #

Patch Set 3 : drop table from lexer class #

Patch Set 4 : unused hdr #

Patch Set 5 : unused hdr #

Patch Set 6 : unused hdr #

Total comments: 1

Patch Set 7 : add missing header #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -148 lines) Patch
D lily/include/keyword.hh View 1 chunk +0 lines, -46 lines 0 comments Download
M lily/include/lily-lexer.hh View 1 2 3 3 chunks +0 lines, -3 lines 0 comments Download
D lily/keyword.cc View 1 chunk +0 lines, -33 lines 0 comments Download
M lily/lily-lexer.cc View 1 2 3 4 5 6 6 chunks +10 lines, -34 lines 0 comments Download
D lily/lily-lexer-scheme.cc View 1 chunk +0 lines, -32 lines 0 comments Download

Messages

Total messages: 22
lemzwerg
LGTM
1 month, 2 weeks ago (2020-04-20 08:14:30 UTC) #1
hahnjo
https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lexer.hh File lily/include/lily-lexer.hh (right): https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lexer.hh#newcode23 lily/include/lily-lexer.hh:23: #include <unordered_map> Please put C++ includes last. I know ...
1 month, 2 weeks ago (2020-04-20 08:17:06 UTC) #2
hanwenn
jonas
1 month, 2 weeks ago (2020-04-20 08:33:53 UTC) #3
hanwenn
https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lexer.hh File lily/include/lily-lexer.hh (right): https://codereview.appspot.com/549920043/diff/553940043/lily/include/lily-lexer.hh#newcode23 lily/include/lily-lexer.hh:23: #include <unordered_map> On 2020/04/20 08:17:05, hahnjo wrote: > Please ...
1 month, 2 weeks ago (2020-04-20 08:34:06 UTC) #4
hahnjo
LGTM, nice cleanup.
1 month, 2 weeks ago (2020-04-20 08:50:08 UTC) #5
hanwenn
drop table from lexer class
1 month, 1 week ago (2020-04-27 08:39:57 UTC) #6
hanwenn
unused hdr
1 month, 1 week ago (2020-04-27 08:41:31 UTC) #7
dak
Without having looked all that much at the code: When the parser sees some \blabla ...
1 month, 1 week ago (2020-04-27 08:59:24 UTC) #8
hanwenn
unused hdr
1 month, 1 week ago (2020-04-27 08:59:46 UTC) #9
hanwenn
unused hdr
1 month, 1 week ago (2020-04-27 09:00:20 UTC) #10
hanwenn
On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote: > > Without having looked ...
1 month, 1 week ago (2020-04-27 09:06:20 UTC) #11
hanwenn
On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote: > When the parser sees ...
1 month, 1 week ago (2020-04-27 09:06:43 UTC) #12
dak
Han-Wen Nienhuys <hanwenn@gmail.com> writes: > On Mon, Apr 27, 2020 at 10:59 AM <dak@gnu.org> wrote: ...
1 month, 1 week ago (2020-04-27 09:11:16 UTC) #13
hahnjo
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#newcode40 ...
1 month, 1 week ago (2020-04-27 10:28:28 UTC) #14
dak
On 2020/04/27 09:11:16, dak wrote: > Han-Wen Nienhuys <mailto:hanwenn@gmail.com> writes: > > > On Mon, ...
1 month, 1 week ago (2020-04-27 11:58:11 UTC) #15
hanwenn
add missing header
1 month, 1 week ago (2020-04-27 20:37:32 UTC) #16
dak
Regarding the "Get rid of unused function ly:lexer-keywords": that one was added with commit 1ae6f5710e714f13da1bfbfbd840835316618d1f ...
1 month, 1 week ago (2020-04-28 10:37:24 UTC) #17
hahnjo
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) ...
1 month, 1 week ago (2020-04-30 07:42:16 UTC) #18
dak
On 2020/04/30 07:42:16, hahnjo wrote: > On 2020/04/27 11:58:11, dak wrote: > > Tracker issue: ...
1 month, 1 week ago (2020-04-30 07:58:09 UTC) #19
hanwenn
On 2020/04/30 07:58:09, dak wrote: > On 2020/04/30 07:42:16, hahnjo wrote: > > On 2020/04/27 ...
1 month ago (2020-05-01 11:15:29 UTC) #20
dak
On 2020/05/01 11:15:29, hanwenn wrote: > On 2020/04/30 07:58:09, dak wrote: > > On 2020/04/30 ...
1 month ago (2020-05-01 11:19:28 UTC) #21
hahnjo
1 month ago (2020-05-01 11:22:12 UTC) #22
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b