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

Rename thread_rng to ThreadRng::new()? #404

Closed
vks opened this issue Apr 17, 2018 · 7 comments
Closed

Rename thread_rng to ThreadRng::new()? #404

vks opened this issue Apr 17, 2018 · 7 comments
Labels
E-question Participation: opinions wanted
Milestone

Comments

@vks
Copy link
Collaborator

vks commented Apr 17, 2018

This would be more consistent with OsRng, EntropyRng, JitterRng, ReseedingRng and BlockRng.

@dhardy
Copy link
Member

dhardy commented Apr 17, 2018

I do not support this. Calling thread_rng has low overhead (just creating another reference) and could be optimised to just a check and pointer copy. The ThreadRng type is just a handle to another object; unfortunately this is not obvious from the name so ThreadRng::new sounds like it is doing a lot more work than merely creating a handle.

OsRng is also just a handle really and I did think about replacing its new with e.g. os_rng, but didn't see much point.

@vks
Copy link
Collaborator Author

vks commented Apr 17, 2018

I don't share that sentiment, new() is just the constructor-by-convention. It is often very lightweight, though not necessarily so. To me it seems like the constructor for ThreadRng (thread_rng) has a non-idiomatic name.

ThreadRng::new sounds like it is doing a lot more work than merely creating a handle.

It is doing a lot of work if it is the first call in the thread.

OsRng is also just a handle really

That is my point, right now we are inconsistent.

@pitdicker
Copy link
Contributor

I wouldn't remove thread_rng(), it is used too much. But adding ThreadRng::new() seems harmless to me.

@dhardy
Copy link
Member

dhardy commented Apr 17, 2018

Having both will just lead to pointless style wars.

@dhardy
Copy link
Member

dhardy commented May 3, 2018

I agree that we shouldn't simply have thread_rng() -> impl Rng because this prevents users from embedding the type in structs, (which may be useful even though it isn't common usage); hence ThreadRng::new() does make intuitive sense, so this may be a good option.

See also this point about replacing ThreadRng with perhaps two alternatives: StrongRng and WeakRng.

In any case, I think we should postpone these ideas until after the 0.5 release.

@dhardy
Copy link
Member

dhardy commented Jun 15, 2018

Since there do appear to be quite a few people who watch this repository and this is mostly style, can I get some more opinions on this, i.e. do you prefer writing thread_rng() or ThreadRng::new()? We could also have both, but it is redundant.

Either way, we already have the ThreadRng type (fn thread_rng() -> ThreadRng).

@dhardy dhardy mentioned this issue Jun 27, 2018
28 tasks
@dhardy dhardy added the E-question Participation: opinions wanted label Aug 1, 2018
@dhardy
Copy link
Member

dhardy commented Aug 1, 2018

There is almost no interest on this, so I guess we can just forget it.

@dhardy dhardy closed this as completed Aug 1, 2018
@dhardy dhardy added this to the 0.6 release milestone Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

No branches or pull requests

3 participants