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 ConcurrentQueue.isEmpty #1012

Merged
merged 8 commits into from Oct 5, 2019

Conversation

@pk044
Copy link
Contributor

pk044 commented Sep 7, 2019

Closes #978

pk044 added 2 commits Sep 7, 2019
Copy link
Collaborator

Avasil left a comment

Thanks for another PR @pk044 !

Could you also add isEmpty for AsyncQueue so API is consistent?

@@ -277,6 +277,15 @@ final class ConcurrentQueue[F[_], A] private (
notifyProducers()
}

/** Checks if the queue is empty.
*
* '''UNSAFE PROTOCOL:''' Concurrent shared state changes very frequently, therefore this function might yield nondeterministic results.

This comment has been minimized.

Copy link
@Avasil

Avasil Sep 9, 2019

Collaborator

I think it could use a new line. Also maybe similar warning to tryPoll would be more accurate, i.e. it implies an understanding of concurrency for given use case. I don't have any good idea about wording off the top of my head :D

Any ideas @alexandru ?

This comment has been minimized.

Copy link
@pk044

pk044 Sep 9, 2019

Author Contributor

ok, i wrote it in the manner you suggested, do you think it's correct more or less/should I add something more?

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Sep 16, 2019

@pk044 this looks good, but can you add some tests? Nothing fancy.

Also, wouldn't this make sense for ConcurrentChannel too? Probably on the consumer side. If yes, that could be a different PR.

@pk044

This comment has been minimized.

Copy link
Contributor Author

pk044 commented Sep 26, 2019

could you guys see if the test I've just added is enough?

I'll make another PR for ConcurrentChannel soon too.

pk044 added 2 commits Sep 26, 2019
…ntqueue-isempty
@Avasil Avasil merged commit a7d2711 into monix:master Oct 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Avasil

This comment has been minimized.

Copy link
Collaborator

Avasil commented Oct 5, 2019

Thanks @pk044

pk044 added a commit to pk044/monix that referenced this pull request Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.