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

Match the behavior of peek_slice to recv_slice #834

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

hunhoffe
Copy link
Contributor

Previously, peek_slice() did not match the behavior of recv_slice() when there is a buffer wrapparound. I believe that by using the read_allocated() method, this will be fixed. I created an issue for this here: #833

This is my first time trying to contribute to smoltcp so I've only tested locally a few ways. One is running my program that uses peek_slice() that previously failed (it now succeeds). I also ran:

cargo +stable test --no-default-features --features alloc,socket-tcp,proto-ipv4,medium-ethernet

And the tests completed succesfully. Please let me know if I need to do anything else.

@Dirbaio
Copy link
Member

Dirbaio commented Aug 22, 2023

Looks good! Can you add a test asserting the new behavior? ie test that it does return data past the wraparound.

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #834 (953ee32) into main (01e1acb) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   79.52%   79.58%   +0.05%     
==========================================
  Files          78       78              
  Lines       27762    27809      +47     
==========================================
+ Hits        22078    22131      +53     
+ Misses       5684     5678       -6     
Files Changed Coverage Δ
src/socket/tcp.rs 96.62% <100.00%> (+0.13%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

@Dirbaio Dirbaio enabled auto-merge August 23, 2023 00:02
@Dirbaio Dirbaio added this pull request to the merge queue Aug 23, 2023
Merged via the queue into smoltcp-rs:main with commit 28a5dd1 Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants