Blockrng: template over element type

[dhardy]

#281 merged with master plus one extra commit.

It is annoying that one must now specify e.g. BlockRng::<u32, ChaChaCore> when the u32 should be deducible, but this way we allow other element types and generic reseeding code over the element type (except for one thing I couldn't get quite right).

[dhardy]

@pitdicker I should say that I re-reviewed at the same time and I'm happy to merge this.

I don't really mind if you prefer not to template over the element type, but I don't think specialisation will allow other element types without this.

[pitdicker]

Thank you! I look forward to seeing what improvements you made.

Github has some trouble showing me what changed in the last commit. Can you please make two separate commits, one the merge with master and one your changes?

[dhardy]

Ugh, yes, I forgot to commit the merge before modifying. I'll try but otherwise you might have to fetch and use git diff.

[dhardy]

That should do it. I actually had to use patch to apply the changes after merging; git rebase simply dropped my changes.

[pitdicker]

Thanks, and looks good!

Do you think your changes do the trick to make BlockRng work with specialization?

[dhardy]

To be honest I don't fully understand specialisation, but your definition:

pub struct BlockRng<R: BlockRngCore<u32>> { ... }

only allowed the type to exist for u32 elements. With the extra parameterisation, specialisation isn't needed: you can go ahead and impl<R: BlockRngCore<u64>> RngCore for BlockRng<u64, R> now if you like; the only catch is that the impl of RngCore for ReseedingRng will need to be repeated for u64.

[pitdicker]

Even better. Do you think it is time to merge?

[burdges]

I asked this elsewhere but I'd think replacing T with associate type Item plays nicer with specialization because it ensures only one such type exists.

[pitdicker]

I once did a try with an associated type, but couldn't really get it to work. Can't remember the details though, and you two are much more knowledgeable... pitdicker@bf44bb1

[dhardy]

@burdges in response to your notes..

I tried making Item an associated type. This makes user code (like for ChaChaRng) cleaner due to one less parameter and avoids the need for phantom data in ReseedingCore. I like it, but no real functional change.

A surprising consequence of the above is that we need to both specify the Item = u32 type and specify that Results: AsRef<[u32]>; I don't understand why the compiler can't prove the second bound itself.

After the above, I tried making the impl of RngCore for BlockRng flexible over the item type. This is more difficult. With an additional constraint, where <R as BlockRngCore>::Item: Into<u32>, the next_u32 implementation is easy (use value.into()). The next_u64 implementation is more difficult; if we are going to use specialisation we need a generic implementation which works, and I'm not really sure how to make this code both an optimal implementation for u32 and a generic implementation for any type given the above bound. We would probably have some trouble with fill_bytes too, since this calls fill_via_u32_chunks which calls to_le() on each element.

Note that we don't need to use specialisation at all; we can just have two separate impls, one for each Item type (u32 and u64).

What we could do instead, once specialisation is available, is have a generic implementation with unimplemented!() members, then specialise for both u32 and u64. The advantage is that impl RngCore for ReseedingRng doesn't need any bounds to work, but overall I don't like this.

Allowing BlockRngCore: ?Sized is very easy to implement. I don't know if there are any drawbacks; I don't believe the compiler does any element reordering yet.

[burdges]

At worst, some private helper trait BlockRngItem satisfied by u32 and u64 makes this work, right?

In any case, an associated type Item prevents MyRng: BlockRng<u32>+BlockRng<u64>, which simplifies doing RngCore generically but even not generically over Item.

[dhardy]

@pitdicker has a point; it would be nice to have this merged! I'll try the BlockRngItem trait idea, but I have a feeling there will be a complication: how to implement RngCore for BlockRng for each Item type separately.

[pitdicker]

🎉