Skip to content

Make Range fields public so others can impl SampleRange#114

Closed
walruscow wants to merge 1 commit intorust-random:masterfrom
walruscow:master
Closed

Make Range fields public so others can impl SampleRange#114
walruscow wants to merge 1 commit intorust-random:masterfrom
walruscow:master

Conversation

@walruscow
Copy link

This allows external types to implement SampleRange and use Range. Currently it is impossible to implement construct_range because the fields are private and a Range struct cannot be created.

Alternative approaches would be to create another new() function on Range, or to not export SampleRange & co.

This allows external types to implement SampleRange and use Range.
Currently it is impossible to implement construct_range because the
fields are private and a Range struct cannot be created.
@bhickey
Copy link

bhickey commented Aug 26, 2016

I think we should avoid leaking the internals of Range, especially implementation details like accept_zone (which is eliminated in PR #115).

@walruscow
Copy link
Author

From the look of it, #115 really just renames accept_zone, but could actually remove it (leading_zeros() oughtn't to be an expensive computation to perform per-call).

I agree that my approach here seems wrong (mostly due to the presence of accept_zone), but it enables this range feature to interoperate with other crates, which appears to be the original intention.

I wanted to reuse this crate's random range interface for another integer type I have created, just like I can use the rng.gen() function. I assumed that was intended, since SampleRange is publicly exposed, and the documentation mentions other types.

Is this inconsistency an oversight or a technical limitation of the language?

@walruscow walruscow closed this Sep 16, 2016
@aldanor
Copy link

aldanor commented Feb 6, 2017

Was also wondering what's the intended way of implementing SampleRange for user types.

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