-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify generated code #61
Conversation
tiny_markdown example fails otherwise. It were broken with mine cf8f511 "add more tests to position handling and fix them"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions mostly!
lib/kpeg/compiled_parser.rb
Outdated
def get_byte | ||
def get_byte(char_range = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only call get_byte
with a range one place, seems better to just have a separate method for that, perhaps check_byte_in_range
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do.
Looks like it were single place where result of get_byte were used. So get_byte
should be also renamed.
match_dot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed. match_dot
is best.
lib/kpeg/code_generator.rb
Outdated
code << indentify(" break unless _tmp\n", indent) | ||
code << indentify(" end\n", indent) | ||
code << indentify(" _tmp = true\n", indent) | ||
code << indentify("_tmp = #{cnt} >= #{op.min} || nil\n", indent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why include the || nil
here? _tmp being false is totally fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did it with NotPredicate: tmp = tmp ? nil : true
. I thought falsey tmp should be nil
.
Ok, it will simplify things.
code << indentify("while true # sequence\n", indent) | ||
code << indentify("begin # sequence\n", indent) | ||
op.ops.each_with_index do |n, idx| | ||
output_op code, n, (indent+1) | ||
|
||
if idx == op.ops.size - 1 | ||
code << indentify(" unless _tmp\n", indent) | ||
code << indentify(" self.pos = #{ss}\n", indent) | ||
code << indentify(" end\n", indent) | ||
code << indentify(" break\n", indent) | ||
else | ||
code << indentify(" unless _tmp\n", indent) | ||
code << indentify(" self.pos = #{ss}\n", indent) | ||
code << indentify(" break\n", indent) | ||
code << indentify(" end\n", indent) | ||
if idx > 0 | ||
code << indentify(" break unless _tmp\n", indent) | ||
end | ||
output_op code, n, (indent+1) | ||
end | ||
code << indentify("end while false\n", indent) | ||
code << indentify("unless _tmp\n", indent) | ||
code << indentify(" self.pos = #{ss}\n", indent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great cleanup.
code << indentify("#{ss} = self.pos\n", indent) | ||
code << indentify("while true # choice\n", indent) | ||
code << indentify("begin # choice\n", indent) | ||
op.ops.each_with_index do |n,idx| | ||
output_op code, n, (indent+1) | ||
|
||
code << indentify(" break if _tmp\n", indent) | ||
code << indentify(" self.pos = #{ss}\n", indent) | ||
if idx == op.ops.size - 1 | ||
code << indentify(" break\n", indent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally get the logic behind not restoring the save here, but we should double check that all the rules, if they consume input, perform the reset themselves. This was always a bit of a failsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think double check pays the bill here. Rules are thoroughly tested by using them within compiller itself. "What could go wrong?" :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! fair enough. You're right through, the metacircularity is the best test bed.
- rename `get_byte`->`match_dot`, add `match_char_range` - abuse choice variants reset position by themself - abuse optional rule reset position by itself - simplify sequence with resetting position after block instead of after each element - unify `+` and numbered repetition - strip actions body Also fix issue with nested repetition collection: - names `_ary` and `_count` were erroneously reused
43f8d84
to
0c3d2fd
Compare
Thanks a bunch! |
get_byte
accept char range+
and numbered repetitionAlso fix issue with nested repetition collection:
_ary
and_count
were erroneously reusedBranch is made on top of #60, so first 2 commits are the same.
Obsoletes #59