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

cloneState/restoreState reuses state if no keys have been set #73

Closed d4l3k closed 6 years ago

d4l3k commented 6 years ago

This test fails since the same empty state is returned from cloneState multiple times.

func TestState(t *testing.T) {
    var p parser
    p.emptyState = make(storeDict)
    p.cur.state = make(storeDict)
    p.restoreState(p.cloneState())
    c := &p.cur

    backup := p.cloneState()

    c.state["foo"] = true

    p.restoreState(backup)
    if len(p.cur.state) > 0 {
        t.Fatalf("leaking state! %#v", p.cur.state)
    }
}
breml commented 6 years ago

@d4l3k thanks for the issues and the pr. I am currently on vacation and I will have a look when I am back.

d4l3k commented 6 years ago

Do you know when that'll be? Hope you're having a good vacation!

On Sun, Jul 29, 2018, 00:47 Lucas Bremgartner notifications@github.com wrote:

@d4l3k https://github.com/d4l3k thanks for the issues and the pr. I am currently on vacation and I will have a look when I am back.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mna/pigeon/issues/73#issuecomment-408653828, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3fMFulcV2loN6CVAm28MbONOfmkEZcks5uLUx8gaJpZM4VlRSX .

breml commented 6 years ago

I am away for an other week.

breml commented 6 years ago

@d4l3k Now I am back from my vacation and I looked at this issue as well as your PR #74. I understand the change you are proposing and also your test. Do you have an actual PEG grammar, that suffers from this particular problem? Do you mind to add a test case including such a PEG grammar, where to problem is shown when the generated parser is acutally used? I would prefer to have a test case, where the exposed API is used instead of directly call the private methods cloneState and restoreState directly. Also I would prefer to have the formatting changes you performed to have in a separate commit.

d4l3k commented 6 years ago

I do have a PEG grammar that suffers from this problem which is how I found the bug.

https://github.com/d4l3k/wikigopher/blob/master/wikitext/wikitext.peg

It's pretty large though and seemed easier to write a unit test than create a stripped down version of it.

Formatting changes happened automatically sorry. I have gofmt set to run every time I save a file.