Repository navigation
Conversation
^ Conflicts: ^ internal/generator/infix_generator.go ^ internal/languages/completion/parser_gen.go
e702d44 to
69e1194
Compare
sailingKieler
left a comment
There was a problem hiding this comment.
Valuable refactoring, thanks Mark! Looks basically good to me.
However, please spend some effort in adding a bit more documentation, see my comments below.
Besides, I'm wondering whether it will be worth the effort of creating parser (... and lexer) generator tests, mainly for the sake of documenting the semantics the generators are implementing.
| for len(f.ops) > frame.ops { | ||
| top := f.ops[len(f.ops)-1] | ||
| if top.level < level || (top.level == level && !right) { | ||
| f.reduce(build) |
There was a problem hiding this comment.
The implementation(s) here and below makes sense completely.
However, I would appreciate to see the assumptions at the crucial points written down as comments, like
// assumption (before the loop): there're at least two preceding operands and one preceding operator on the stacks;
// if the previous operator is of lower level (higher precedence?) then the current 'token' reduce it eagerly at this point.
// do the same if the former op is of the same precedence and the current op is right assoziative
// assertion (after the loop): all remaining operators on the stack must not reduced right now due to lower precedence, right associativity, and/or belonging to outer rule calls
| } else { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
// finally push the current operator
| ops = append(ops, stackedOp{token, level}) | ||
| operands = append(operands, operand(i+1)) | ||
| for len(f.ops) > frame.ops { | ||
| f.reduce(build) |
There was a problem hiding this comment.
// assumption (before the loop): there'are at least two preceding operands and one preceding operator on the stacks that need(s) to be resolved in right associative manner;
// resolve them now
// assertion (after the loop): all remaining operators on the stack must not reduced right now due to belonging to outer rule calls, there's at least one operand on the stack representing the result of this rule call, may be `zero` in case operands and ops were pushed
| ops []infixOp | ||
| } | ||
|
|
||
| // InfixFrame marks the stack depths at the start of one infix rule invocation. |
There was a problem hiding this comment.
Please elaborate a bit on the purpose of this, like
// tracks the stack positions of the current state once another recursive invocation is treated via transitive re-call the of same rule, like
// PrimaryExpression returns Expression: "(" BinaryExpression ")" | ...
What about InfixBase as an alternative name?
|
|
||
| func isDeferrableKeyword(element grammar.Element) bool { | ||
| _, ok := element.(grammar.Keyword) | ||
| return ok && element.Cardinality() == CardinalityOne |
There was a problem hiding this comment.
What about CardinalityOptional here?
Would that be too error-prone? Did you just restrict on the CardinalityOne to be on the safe side?
Please add a comment on that.
In an attempt to improve parser performance of the arithmetics language, this change defers the allocation of AST even further than previously. For example, we previously already deferred the allocation on parser rules like
X: A | B;, sinceXdidn't ever generate an AST node. Now, we can also do it for rules such asX: '(' A ')' | B;.Additionally, reduces the need for allocations during the infix parsing process, by reusing a
InfixFoldstructure. Speeds up the arithmetics parser by 50% in total (reduces memory allocation amount by 33%).