search

mna / pigeon

Command pigeon generates parsers in Go from a PEG grammar.
BSD 3-Clause "New" or "Revised" License
835 stars 66 forks source link

Fix duplicate function code generation #66

Closed mna closed 6 years ago

mna commented 6 years ago

This closes #65 .

After optimization with --optimize-grammar, a single action (or other code-block-associated expression) AST node can be used in more than one location. As a result, it gets assigned conflicting FuncIx values and, because the builder first generates the grammar AST tree and then generates the code block, the functions get generated more than once with the same name, resulting in build failures on the generated code.

This PR makes sure that a given AST node never gets its FuncIx overwritten (avoiding the (*parser).callonList4 undefined type of errors for the initial FuncIx that was subsequently overwritten), and makes sure to render a given function only once (to avoid the (*current).onList10 redeclared in this block type of errors).

Another approach could be to always clone nodes when optimizing, never assigning any reference, but that would result in code bloat (e.g. the same function body rendered multiple times under different names), and would possibly make the optimization code more complex.

mna commented 6 years ago

Looks like the tests < Go1.7 all fail because the imports package now uses the context package from the stdlib, which was introduced in Go 1.7. I can update the travis CI file in this PR if you'd like.

breml commented 6 years ago

@mna Thank you very much for the bug report #65 and this PR to resolve it.

I updated .travis.yml and README.md in regards to the Go < v1.7. (please rebase this PR)

I also reviewed this PR and it makes sense to me. I only have two minor suggestions:

  1. Because the bug only shows up, if -optimize-grammar is used, I suggest to run the tests with and without optimization (similar to how this is done in examples/json).
  2. Because this test case handles an issue (#65) I suggest to place this test case in the directory test/issue_65 (similar to how this was done for issues #1 and #18).
mna commented 6 years ago

Sounds good to me, I should be able to address your comments on Thursday. Thanks!

Martin

mna commented 6 years ago

@breml I addressed your comments, will let you take another look and merge (+squash if you want) if it looks good to you.

breml commented 6 years ago

@mna Thank you very much.