Closes #446

[ethanpailes]

Thanks @BurntSushi for doing all the legwork on cutting down the failing test case and laying out a clear set of execution steps. It really helped me zero in on the bug.

This patch fixes an issue where skip resolution would go strait
to the default value (the md2_shift) on a match failure after
the shift_loop. Now we do the right thing, and first check in
the skip table. The problem with going strait to the md2_shift
is that you can accidentally shift to far when window_end
actually is in the pattern (as is the case for the failing
match).

In the issue thread I promised better modularity, but it turns
out that shift resolution was a bit too decomposed in the other
places I had mentioned. Sorry :(.

[ethanpailes]

Local testing: I ran the unit tests both at just this commit and with TBM turned back on. I'm currently running the quickcheck tests in a loop.

[BurntSushi]

Awesome! Thanks so much for such a quick fix. :-)

[BurntSushi]

I'm currently running the quickcheck tests in a loop.

Did you see my comment about the quickcheck tests? Frankly, I'm surprised you got as much mileage out of them as you did. :-)

[ethanpailes]

Yeah, I agree about the quickcheck tests not being that likely to catch any remaining bugs. I'm only running them because I already have a hammer, so why not thrash about to try to find a nail that this change could have turned up.

I think the principled thing to do would be to hook them into a fuzzer somehow? Those have an actual model of the state space and tweak the input to more fully exersise it right?

[ethanpailes]

The custom arbitrary instance you suggest should definitly be part of the picture as well.

[BurntSushi]

I don't have much experience with fuzzers, but with an Arbitrary instance, you can encode whatever model of state you choose. :)

[ethanpailes]

I think fuzzers do some code coverage deep magic, but I also don't really have experience with them.

[ethanpailes]

@BurntSushi bump

[BurntSushi]

@ethanpailes Thanks again!