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

Add Lwt_mvar.(take_available, is_empty) #459

Merged
merged 5 commits into from Jul 27, 2017

Conversation

hcarty
Copy link
Contributor

@hcarty hcarty commented Jul 27, 2017

Closes #443

Factoring out next_writer as common (internal) functionality.
Use Lwt_mvar.take_available as a base to avoid a bit of duplicate logic.
@hcarty
Copy link
Contributor Author

hcarty commented Jul 27, 2017

I've split the PR into a handful of commits to show the steps taken. If you're happy with the implementation feel free to merge as-is or squash on merge.

@hcarty hcarty mentioned this pull request Jul 27, 2017
let x = Lwt_mvar.create 0 in
let y = Lwt_mvar.take_available x in
Lwt.return (y = Some 0)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be another test to check that after take_available has been called, the mvar is empty. Something along the lines of:

test "take_available (twice)"
(fun () ->
  let m = Lwt_mvar.create 0 in
  let (_: int option) = Lwt_mvar.take_available m in
  let t = Let_mvar.take_available m in
  Lwt.return (t = None)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, thanks! Added in 7799c15

@aantron
Copy link
Collaborator

aantron commented Jul 27, 2017

It looks good. Thanks @hcarty for the PR, @raphael-proust for the insightful review, and @jsthomas for writing the tests for existing Lwt_mvar that made reviewing this PR way easier than it would be otherwise.

I restarted the stuck AppVeyor build. Will merge once that is finished.

@hcarty
Copy link
Contributor Author

hcarty commented Jul 27, 2017

Yes, big thanks to @jsthomas! Your work made adding these tests very very simple.

@aantron aantron merged commit bc987a9 into ocsigen:master Jul 27, 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.

None yet

3 participants