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

Add DistString trait #1133

Merged
merged 4 commits into from Jun 14, 2021
Merged

Add DistString trait #1133

merged 4 commits into from Jun 14, 2021

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jun 7, 2021

Closes #1128. Review @vks, @abonander.

Thoughts? Not quite sure myself; it may be more natural to have rng.gen_string(Alphanumeric, 20).

src/distributions/other.rs Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator

vks commented Jun 7, 2021

Looks good to me. We might want to generalize this to allow bulk generation of vectors. Such an API might benefit more from SIMD than our current one.

@dhardy
Copy link
Member Author

dhardy commented Jun 8, 2021

We might want to generalize this to allow bulk generation of vectors.

What did you have in mind? A String is not a Vec<u8>.

My understanding is that the unique challenge here is that asserting a bunch of bytes is a valid UTF-8 String is unsafe or requires a (redundant) test.

@vks
Copy link
Collaborator

vks commented Jun 8, 2021

I guess I wanted to unify this with the Fill API. Anyway, this is something that can be done later.

@dhardy
Copy link
Member Author

dhardy commented Jun 8, 2021

You mean like try_fill_with_length(&mut self, rng: &mut R, len: usize) ?

How would len be interpreted in the case of a String? It usually refers to the byte-length.

@vks
Copy link
Collaborator

vks commented Jun 8, 2021

I think we would still need a different trait for strings, for the reason you mentioned, I just think the interfaces could be more uniform.

@dhardy
Copy link
Member Author

dhardy commented Jun 8, 2021

It's already possible to do something like this:

let mut v: Vec<f32> = iter::repeat(f32::NAN).take(20);
use rand::Fill;
v[..].try_fill(&mut rand::thread_rng())?;

That still requires initialisation but skipping that is #1080 ... we're way off topic.


Anyway, we can wait a couple of days to give @abonander and @qoh a chance to review this if they're interested. Then it's time to review the change-log for the next patch release.

src/distributions/other.rs Outdated Show resolved Hide resolved
src/distributions/other.rs Outdated Show resolved Hide resolved
src/distributions/other.rs Outdated Show resolved Hide resolved
@abonander
Copy link

@dhardy This looks fine to me but I thought you preferred to keep the interface simple?

@dhardy
Copy link
Member Author

dhardy commented Jun 10, 2021

It's one trait plus two impls, so reasonably simple.

@TheIronBorn
Copy link
Collaborator

Looks good to me. We might want to generalize this to allow bulk generation of vectors. Such an API might benefit more from SIMD than our current one.

I'm working on a SIMD alphanumeric append_string if that covers 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.

Implement Alphanumeric::gen_string
5 participants