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

Distributions without direct dynamic dispatch #261

Closed
wants to merge 14 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Feb 15, 2018

This is #256 without #244, i.e. Distribution::sample takes rng: &mut R where R: Rng, not R: Rng + ?Sized.

The advantage of this approach is that it doesn't require #244, which in some ways is less ergonomic for users.

The disadvantages are

  1. less ergonomic support for using trait object Rngs
  2. if we wanted to add the equivalent of Add trait SampleRng: Rng and related doc #244 + Replace distribution::Sample with Distribution + polymorphism over Rng #256 later, it would break all existing Distribution implementations

if mem::size_of::<isize>() == 4 {
rng.gen::<i32>() as isize
} else {
rng.gen::<i64>() as isize
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about platforms with 16 bit pointers? Does Rust support them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. There was some discussion of 16-bit CPUs here but ultimately because of the extremely limited memory addressing I think most 16-bit code has to be written expressly for that architecture.

@vks
Copy link
Collaborator

vks commented Feb 16, 2018

Looks good to me!

The advantage of this approach is that it doesn't require #244, which in some ways is less ergonomic for users.

Could you give some example user code contrasting the two approaches?

@dhardy
Copy link
Member Author

dhardy commented Feb 16, 2018

Could you give some example user code contrasting the two approaches?

With this PR:

use rand::{thread_rng, Rng};
use rand::distributions::Uniform;

let mut rng = thread_rng();
let mut erased_rng: &mut Rng = &mut rng;
let val: f32 = (&mut erased_rng).sample(Uniform);
println!("f32 from [0,1): {}", val);

With #256 instead:

use rand::{thread_rng, Rng, SampleRng};
use rand::distributions::Uniform;

let mut rng = thread_rng();
let mut erased_rng: &mut Rng = &mut rng;
let val: f32 = erased_rng.sample(Uniform);
println!("f32 from [0,1): {}", val);

As you can see, very little difference — the extra reference to erased_rng vs the extra import (SampleRng). The most significant user difference perhaps is that the former solution is not obvious.

@vks
Copy link
Collaborator

vks commented Feb 16, 2018

Thanks! Why is the extra &mut needed? What is the compiler error if you miss it?

Edit: I missed what you said in #244:

rng.gen() will not work with rng: &mut Rng, although (&mut rng).gen() appears to work — I don't understand why the latter works

@dhardy
Copy link
Member Author

dhardy commented Feb 16, 2018

See this comment.

From the error message one wouldn't expect to be able to use generic methods on a type-erased Rng at all; we can add documentation there but of course people may miss it.

        error: the `sample` method cannot be invoked on a trait object
 --> src/distributions/mod.rs:9:27
  |
9 | let val: f32 = erased_rng.sample(Uniform);
  |                           ^^^^^^

@vks
Copy link
Collaborator

vks commented Feb 16, 2018

If I understand it correctly, this is caused by this:

impl<'a> Rng for &'a mut Rng { ... }

Why is this impl necessary?

@dhardy
Copy link
Member Author

dhardy commented Feb 16, 2018

Without that impl, the indirect dispatch (&mut rng).gen() won't work either with type-erased Rng.

@burdges
Copy link
Contributor

burdges commented Feb 16, 2018

Also, one writes struct Foo<R: Rng> { rng: R, ... } like everywhere, specifically so that Foo<SomeRng> and Foo<&mut SomeRng> both work.

@dhardy
Copy link
Member Author

dhardy commented Feb 19, 2018

This is obsolete now that I've merged #265.

@dhardy dhardy closed this Feb 19, 2018
@dhardy dhardy deleted the distr-static branch February 19, 2018 15:34
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