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

Issue 41720043: Parser: make optional arguments compatible with lookahead (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by dak
Modified:
10 years, 2 months ago
Reviewers:
lemzwerg
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Parser: make optional arguments compatible with lookahead Basically reimplementing the MYBACKUP and MYREPARSE macros based on an experimentally verified understanding of the LALR(1) parser produced by Bison, this makes it possible to greatly simplify the grammar while making optional funciton arguments (and particularly the following non-optional arguments even when optional arguments are being skipped) behave much more predictable. Further simplifications are feasible: this is a first pitch. Consists of the following commits in reverse order: Adapt documentation for optional arguments now that lookahead is permissible Parser: properly disambiguate pitches from music in function arguments In many instances, function arguments were parsed greedily without looking at the function argument predicates first. This did not allow using subsequent function arguments of type pitch and duration since they were combined into one music expression. Parser: remove command_element and command_event synonyms of tempo_event Parser: remove pitch_arg and PITCH_ARG Parser: inline a few "common" expressions occuring only once now Remove all closed music expressions Permit event functions to use a non-closed argument list. Add location data to extra_tokens_ stack in lexer. Parser: let MYREPARSE and MYBACKUP back up even in presence of lookahead This implies that the lookahead token must not yet have made an impact on the state stack other than causing the reduction of the current rule, or switching the lookahead token while Bison is mulling it over will cause trouble. Since an LALR(1) works on the top of the stack, this can usually be guaranteed. The "Too much lookahead" message is not produced. If these constructs are misused, the parser should typically complain about the replacement token being unexpected. The advantage is the ability to vastly simplify syntax rules that up to now have to avoid lookahead tokens with a lot of drudgework. Move the extra_token mechanism out of the lexer proper Calling lex->pop_extra_token explicitly in the parser's yylex function makes things considerably more robust. It also allows pushing and popping the EOF condition, necessary for backing up right at the end of a file, like with ##{ \relative { c d e f } #} where the music expression is checked against being a pitch, and then is backed up.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+258 lines, -449 lines) Patch
M Documentation/extending/programming-interface.itely View 2 chunks +3 lines, -9 lines 0 comments Download
M lily/include/lily-lexer.hh View 2 chunks +3 lines, -2 lines 0 comments Download
M lily/lexer.ll View 10 chunks +29 lines, -70 lines 0 comments Download
M lily/lily-parser.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/parser.yy View 42 chunks +222 lines, -367 lines 3 comments Download

Messages

Total messages: 4
lemzwerg
As usual, thanks for working on such simplifications and clean-ups! Inspecting the code (without really ...
10 years, 4 months ago (2013-12-13 06:13:32 UTC) #1
dak
https://codereview.appspot.com/41720043/diff/1/lily/parser.yy File lily/parser.yy (right): https://codereview.appspot.com/41720043/diff/1/lily/parser.yy#newcode1073 lily/parser.yy:1073: if (!unsmob_music ($$)) On 2013/12/13 06:13:32, lemzwerg wrote: > ...
10 years, 4 months ago (2013-12-13 06:35:19 UTC) #2
lemzwerg
https://codereview.appspot.com/41720043/diff/1/lily/parser.yy File lily/parser.yy (right): https://codereview.appspot.com/41720043/diff/1/lily/parser.yy#newcode1073 lily/parser.yy:1073: if (!unsmob_music ($$)) > What's more annoying is that ...
10 years, 4 months ago (2013-12-13 06:47:29 UTC) #3
dak
10 years, 4 months ago (2013-12-13 06:57:13 UTC) #4
On 2013/12/13 06:47:29, lemzwerg wrote:
> https://codereview.appspot.com/41720043/diff/1/lily/parser.yy
> File lily/parser.yy (right):
> 
> https://codereview.appspot.com/41720043/diff/1/lily/parser.yy#newcode1073
> lily/parser.yy:1073: if (!unsmob_music ($$))
> > What's more annoying is that the copy&paste passages often use spaces
> > instead of tabs, while the global indentation style is supposed to use
> > tabs.
> 
> This is what I'm referring to.  I find such issues irritating, but maybe this
is
> only me.
> 
> > The inconsistency is somewhat annoying, but in the interest of letting
> > "git blame" work, gratuitous changes while moving lines around are not
> > a good idea.
> 
> Yep.  There are more such issues in the file, BTW.  I'm quite unhappy that  we
> can't fix this easily without obfuscating results of `blame' command...

One can "fix" indentation in a separate commit not doing other things.
In the particular case of lily/lexer.ll and lily/parser.yy however,
there is no automatic indentation available.  Indentation Linux with tabs
merely means that you have to manually indent start and end of a syntax
rule and most lines inside of a rule will match that.

Since we have no automatic indentation engine that will work for those files,
it seems pointless to do reorganizations manually that will not stay around.

There is a comment at the start of those files essentially expressing this
rationale.

I have not actively invested thought in the tabs/spaces in this patch: the
Emacs settings here imply that new code will get tab-spaced, so the
space-spacing definitely stems from copy&paste passages.  Where they are
really copy&paste rather than cut&paste, git will likely not correlate
them anyway, but given the overall state of the file, it seems silly to
try making a large effort.
Sign in to reply to this message.

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