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

Fix: BytesStream failing to set correct slice range #1201

Merged

Conversation

dmweis
Copy link
Contributor

@dmweis dmweis commented Feb 8, 2023

BytesStream calls Bytes::slice with incorrect range of START..SIZE. Range should be START..END.

This results in incorrect slicing or even an assert failure of "range start must not be greater than end: coming from the slice methnod.

I wanted to add a test for this but don't see any tests for this code. If there are some I am happy to add a test for this case.

Tested locally from my fork

@dmweis
Copy link
Contributor Author

dmweis commented Feb 9, 2023

Ah. Looks like the failures are because of new clippy version. Not because of my changes.

Should I create a separate PR to fix that?

@demoray
Copy link
Contributor

demoray commented Feb 9, 2023

#1203 should have already addressed the clippy issues. Can you rebase to the latest main?

@cataggar
Copy link
Member

cataggar commented Feb 9, 2023

I'm curious what the impact is and how we get unit tests on it.

@johnbatty
Copy link
Contributor

Fix looks good to me.

@johnbatty
Copy link
Contributor

@dmweis I've written a few unit tests to provide some coverage of these functions (and which fail with the unmodified code/work with your fix). Are you happy for me to push these to your branch?

@dmweis
Copy link
Contributor Author

dmweis commented Feb 28, 2023

@dmweis I've written a few unit tests to provide some coverage of these functions (and which fail with the unmodified code/work with your fix). Are you happy for me to push these to your branch?

Oh! That would be amazing! Please go ahead!

And my appologieze. I should have written tests. I forgot about this PR.

Happy to do any more work if there is anything else you'd like changed?

@johnbatty
Copy link
Contributor

@dmweis Ok, I've added a few unit tests.
@demoray Please could you take another look.

@dmweis
Copy link
Contributor Author

dmweis commented Feb 28, 2023

@microsoft-github-policy-service agree

@demoray demoray merged commit 564f185 into Azure:main Feb 28, 2023
@dmweis dmweis deleted the dmw/bytestream-setting-wrong-end-index-of-slice branch May 11, 2023 22:36
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.

None yet

4 participants