Skip to content

Fix documentation of fns rand_core::le::read_u*_into #1626

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

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Apr 10, 2025

Fixes some documentation, as pointed out in #1624.

I considered deprecating or renaming these fns, but they have some utility and only really need doc fixes.

@dhardy dhardy requested a review from newpavlov April 10, 2025 16:04
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

What about changing the asserts to equality?

@dhardy
Copy link
Member Author

dhardy commented Apr 10, 2025

I would, but that's technically a breaking change.

@newpavlov
Copy link
Member

newpavlov commented Apr 10, 2025

Technically, yes. But I think it's fine since I don't think any downstream users rely on the non-equal case and we could declare that it's a "bug fix" (if we are to stretch the meaning far enough). We had a similar "breaking change" in getrandom: rust-random/getrandom#614.

@vks
Copy link
Collaborator

vks commented Apr 11, 2025

Why do you want to change the assert? I don't really see the harm, any unused source bytes are just ignored.

@dhardy
Copy link
Member Author

dhardy commented Apr 11, 2025

Unused input bytes are a possible indicator of mis-use. But I think it's best not to change the assert now (possibly on the next breaking change, but not important).

@dhardy dhardy merged commit d468501 into master Apr 11, 2025
15 checks passed
@dhardy dhardy deleted the push-ltnylznotyzl branch April 11, 2025 08:47
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.

3 participants