Skip to content

Inconsistency between code and comment #1624

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

Closed
YichiZhang0613 opened this issue Apr 10, 2025 · 6 comments
Closed

Inconsistency between code and comment #1624

YichiZhang0613 opened this issue Apr 10, 2025 · 6 comments

Comments

@YichiZhang0613
Copy link

In rand-master/rand_core/src/le.rs, following comment and code are inconsistent.

/// # Panics
///
/// If `dst` has insufficient space (`4*dst.len() < src.len()`).
#[inline]
#[track_caller]
pub fn read_u32_into(src: &[u8], dst: &mut [u32]) {
    assert!(src.len() >= 4 * dst.len());
...
/// # Panics
///
/// If `dst` has insufficient space (`8*dst.len() < src.len()`).
#[inline]
#[track_caller]
pub fn read_u64_into(src: &[u8], dst: &mut [u64]) {
    assert!(src.len() >= 8 * dst.len());
...
@newpavlov
Copy link
Member

assert! panics if the condition is false, so the function docs are correct.

@newpavlov newpavlov closed this as not planned Won't fix, can't repro, duplicate, stale Apr 10, 2025
@newpavlov newpavlov marked this as a duplicate of #1625 Apr 10, 2025
@newpavlov
Copy link
Member

newpavlov commented Apr 10, 2025

I misread the function doc because of the swapped src and dst. The assert and docs indeed do not match.

@newpavlov newpavlov reopened this Apr 10, 2025
@newpavlov
Copy link
Member

newpavlov commented Apr 10, 2025

@dhardy
It seems the asserts are wrong? They assert that src is bigger than dst. Maybe we should just assert equality here?

Technically, it would be a breaking change, but I don't think it makes sense to sue these functions with buffers not equal to each other byte-wise.

@dhardy
Copy link
Member

dhardy commented Apr 10, 2025

No; the objective is not to copy all of the input but rather to fill dst. For that the assert is correct not the docs. Thanks @YichiZhang0613.

I'm not sure why we don't assert that the input is exactly the right length though.

I also don't much like the name or wording of the docs; I'm tempted to rename to read_u32_slice_from_bytes etc. — or just remove these methods and give a doc example somewhere (it's not much code and apparently only used in SeedableRng::from_seed impls, and already has #[inline]; one of the two methods is used by most PRNGs however).

@newpavlov
Copy link
Member

Well, removing them would certainly be a breaking change, but deprecation sounds good to me.

@dhardy
Copy link
Member

dhardy commented Apr 11, 2025

I fixed the docs

@dhardy dhardy closed this as completed Apr 11, 2025
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

No branches or pull requests

3 participants