Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor implementation of Decode::skip and Decode::encoded_fixed_size #587

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kalaninja
Copy link
Contributor

  • add Input::skip
  • refactor implementation of Decode::encoded_fixed_size
  • refactor implementation of Decode::skip

By default, Input::skip does an actual read and discards the result, implementors should provide a specialized implementation.

By default, Decode::skip executes Input::skip for types providing Decode::encoded_fixed_size. Otherwise, it is just calling Decode::decode., so implementors should provide a specialized implementation.

As discussed in previous attempts that I found on github, Decode::skip shouldn't validate the input data, providing a fast forward functionality instead. IMHO if a calling code wants to check for data integrity they can manually do something like let _ = Decode::decode(input)?;

Closes #208
Closes #244

@kalaninja
Copy link
Contributor Author

@ggwpez @bkchr guys, what do you think about this?

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

When skipping we wouldn't do the descend_ref/ascend_ref. I don't if this is an issue.

I don't know if tests really test for skip, encoded_fixed_size.
Ideally we should have for every types, a bunch of value, with the expected encoded_fixed_size, then we test decode, skip and encoded_fixed_size.

src/bit_vec.rs Outdated Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
@gui1117
Copy link
Contributor

gui1117 commented Nov 15, 2024

I added some tests on your branch

@gui1117
Copy link
Contributor

gui1117 commented Nov 15, 2024

Thinking about descend_ref/ascend_ref. it is mostly used with decode_with_depth_limit.

I think we can call them, or we can just have a comment on the skip specification saying, skip doesn't take into account descent_ref and ascend_ref.

But at a first glance implementing skip with descend_ref/ascend_ref seems not difficult so maybe we should just use them.

WDYT?

@kalaninja
Copy link
Contributor Author

Yes, I agree descend_ref/ascend_ref should be used when skipping. I added a corresponding commit to enable this for implementors of WrapperTypeDecode and everything that is skipped (encoded) as Vec.

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I added a minor test.

@kalaninja
Copy link
Contributor Author

Hey @gui1117
Any plans on merging this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize implementation of Decode::skip() and Decode::encoded_fixed_size() for most primitives types
2 participants