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

added Bat{List|Array}.shuffle #718

Closed

Conversation

UnixJunkie
Copy link
Member

No description provided.

Francois BERENGER added 2 commits February 7, 2017 16:02
heavily inspired by the in-place array shuffle that was found
in BatRandom before
@gasche
Copy link
Member

gasche commented Feb 7, 2017

You should try to install the resulting version of Batteries and load it from a toplevel to run one of the new functions. I suspect that you need to add the new file to the .mllib (and maybe some other place?) for this to work.

(You can update pull-requests by just doing git push --force towards you personal github clone of the repository, no strong need to close a PR to open a new one.)

@UnixJunkie
Copy link
Member Author

I am testing it; I think the experience in the toplevel is not what people want.

@gasche
Copy link
Member

gasche commented Feb 7, 2017

(The failure model I had in mind was not toplevel-specific: if I remember correctly from my own BatConcreteQueue mistakes, compiling a program using Batteries fails at link-time if the mllib is not complete.)

@UnixJunkie
Copy link
Member Author

I changed the semantic and ocamldoc strings.
Please have a look before accepting (eventually).

@gasche
Copy link
Member

gasche commented Feb 7, 2017

I am not convinced by the change: why change away from optional arguments again, is there a good reason for this?

I think the change you want to make is just turning

?(s = Random.get_state ()) ... ->

into

?s ... ->
let s = match s with
| Some s -> s
| None -> Random.get_state ()
in

Edit: I thought the issue was that the default value would not be evaluated on each function call, but in fact it does. So the recommendation in this message is useless as the two programs are equivalent.

@UnixJunkie
Copy link
Member Author

In case no random state was provided, several successive calls to shuffle should move the global
random state forward.
In case a random state was provided, it is moved forward but not given back to the caller, which might be a problem.
I don't know how to detect with an optional argument that the argument was not passed; hence I reverted back to an option type.

@UnixJunkie
Copy link
Member Author

What I describe is also the behavior I observed in Core.List.permute.

@UnixJunkie
Copy link
Member Author

other proposal there: #720

@UnixJunkie
Copy link
Member Author

done by 3c3bc7d

@UnixJunkie UnixJunkie closed this Feb 17, 2017
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.

2 participants