Skip to content

Add DistString trait#1133

Merged
dhardy merged 4 commits into
masterfrom
work2
Jun 14, 2021
Merged

Add DistString trait#1133
dhardy merged 4 commits into
masterfrom
work2

Conversation

@dhardy

@dhardy dhardy commented Jun 7, 2021

Copy link
Copy Markdown
Member

Closes #1128. Review @vks, @abonander.

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

Comment thread src/distributions/other.rs Outdated
@vks

vks commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

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

dhardy commented Jun 8, 2021

Copy link
Copy Markdown
Member Author

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

vks commented Jun 8, 2021

Copy link
Copy Markdown
Contributor

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

@dhardy

dhardy commented Jun 8, 2021

Copy link
Copy Markdown
Member Author

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

vks commented Jun 8, 2021

Copy link
Copy Markdown
Contributor

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

dhardy commented Jun 8, 2021

Copy link
Copy Markdown
Member Author

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.

Comment thread src/distributions/other.rs Outdated
Comment thread src/distributions/other.rs Outdated
Comment thread src/distributions/other.rs Outdated
@abonander

Copy link
Copy Markdown

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

@dhardy

dhardy commented Jun 10, 2021

Copy link
Copy Markdown
Member Author

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

@TheIronBorn

Copy link
Copy Markdown
Contributor

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