fix lists decoding #17
Conversation
|
|
||
| {item, new_tail} | ||
| end | ||
|
|
||
| @spec decode_hex(binary()) :: binary() | ||
| defp decode_hex(binary) do |
There was a problem hiding this comment.
I don't think this is used anywhere?
|
|
||
| item_length = :binary.decode_unsigned(be) | ||
|
|
||
| <<item::binary-size(item_length), new_tail::binary()>> = data | ||
| <<item::binary-size(item_length), new_tail::binary>> = data |
There was a problem hiding this comment.
would there be an option to extract this into a function head?
if you compile this with bin_opt_info flags you get informed that
NOT OPTIMIZED: sub binary is used or returned
you can read more about this here http://erlang.org/documentation/doc-6.0/doc/efficiency_guide/binaryhandling.html
There was a problem hiding this comment.
I don't think so because length is not constant
There was a problem hiding this comment.
what the warning suggest is that it could delay the binary creation if it was passed in a function call.
so instead of returning {item, new_tail} you would return data and pattern match to decompose
|
|
||
| decode_item(new_tail, [next_list | result]) |
There was a problem hiding this comment.
This does remove a tail-call optimization, right?
There was a problem hiding this comment.
no, it's moved to add_decoded_list function
| else | ||
| decode_long_binary(prefix, tail, 247) | ||
| end | ||
| def add_new_item(result, new_item) do |
There was a problem hiding this comment.
I'm not sure if this function is more clear than simply inline the list operation. Mostly, it obscures (in the code above) that this function is just a simple operation and nothing more complicated.
There was a problem hiding this comment.
yes, I agree. But we may have nil in the result variable.
I can change it to
if is_nil(result) do
new_item
else
[new_item | result]
end
in every place where this function is used
We discovered that our implementation of RLP encoding decodes some RLP binaries incorrectly after recent PR (mana-ethereum/ex_rlp#17) but it passes all RLP test cases. This PR adds a new test case which is not covered by existing RLP tests
In the previous PR (#15) tail optimizations were introduced to the decoding of RLP binaries. But ethereum common RLP tests did not cover all cases so a bug was introduced to the library which caused failing tests in mana (https://circleci.com/gh/mana-ethereum/mana/11778?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)
This PR fixes this bug
https:/mana-ethereum/ex_rlp/compare/ab-fix-decoding?expand=1#diff-8eb9d06c2f6b0c5b724e363538ef07c8R25