Bug fix for field index

[erikjmiller]

An error was generated when compiling files that "use Thrash.Protocol.Binary". The only thing I can think of is I am using 1.5.1 which could have change how some of the matching occurs.

** (CompileError) test/simple_struct.ex:14: cannot invoke remote function :erlang.+/2 inside match
(elixir) src/elixir_bitstring.erl:65: :elixir_bitstring.expand_expr/4
(elixir) src/elixir_bitstring.erl:19: :elixir_bitstring.expand/6
(elixir) src/elixir_bitstring.erl:9: :elixir_bitstring.expand/4

After some investigation it looks like we should be evaluating ix + 1 prior to performing any matching, so I changed that and all tests passed.

I'm not entirely sure this is an appropriate fix, though I have tested thrift object (de)serialization and everything works including the existing test cases.

[dantswain]

Thanks @erikmiller ! I'm out of town on vacation. I'll take a look at this sometime next week. I recommend also taking a look at Pinterest/Elixir-thrift - the thrift_tng branch (I'd get a better link but I'm on my phone). That branch hasn't been released yet but I've been using it in production for a while now and it's a pretty easy transition from thrash.

[isaacsanders]

@erikmiller You are a gentleman and a scholar.

@dantswain I would appreciate this, too. I hope you had a good vacation.

[dantswain]

@isaacsanders thanks for the bump - this had honestly slipped my mind :(

@erikmiller Does the test suite reproduce your bug in 1.5.1? I would like to update the build matrix and see if I can replicate this in travis (I'll do a separate PR). Then we can verify that this fixes it.

Also, I strongly recommend taking a look at https://github.com/pinterest/elixir-thrift - the thrift_tng branch currently has the new implementation, but we are very close to merging into master. I know it's not always easy to swap out libraries, though, so I will definitely try to get thrash working for you :)

[erikjmiller]

@dantswain Yeah the bug still occurs in 1.5.1.

[isaacsanders]

@dantswain I hope that means it was a good vacation 😁

Having also implemented the change in my local copy of the package in my deps/ for the project that uses it, the project now compiles with the given change. (We are also on 1.5.1)

[dantswain]

@isaacsanders It was a fantastic vacation, and also a week at ElixirConf :-D

@isaacsanders and @erikmiller can you check out #18 and see if the versions of OTP and Elixir you use are covered? I'm using the newest patch levels for each minor release of Elixir from 1.1 to 1.5. Matching up versions of Elixir and OTP is a bit tricksy.

[erikjmiller]

@dantswain Looks good to me.

[dantswain]

OK, merged this and am running my travis branch one more time. Assuming that passes I'll get it merged and release a hotfix this afternoon. Thanks again guys!

[dantswain]

Sorry for the delay. 0.3.3 published to hex this morning. Thanks for the report and the fix!!