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

Implemented the 'SkipLast' operator #414

Merged
merged 5 commits into from
Oct 9, 2013

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Sep 29, 2013

Hi,

I implemented the skipLast operator #78. I used ReentrantLock and LinkedList to implement it rather than LinkedBlockingDeque like takeLast #85 #140 for two reasons.

  • LinkedBlockingDeque requires that count is greater than 0 but skipLast can accept 0.
  • LinkedBlockingDeque requires the elements can not be null but an observable can emit a null value.

In summary, LinkedBlockingDeque will cause issues like #413.

Please take a look. Thanks.

@cloudbees-pull-request-builder

RxJava-pull-requests #320 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #322 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #326 FAILURE
Looks like there's a problem with this pull request

@samuelgruetter
Copy link
Contributor

another instance of #383

@zsxwing
Copy link
Member Author

zsxwing commented Oct 1, 2013

In addition, though ArrayDeque is more efficient than LinkedList, it also rejects null values. I prefer to an array implementation of Deque since it can avoid to create Entry objects. But now I can not find any appropriate class. Is there any better class?

@benjchristensen
Copy link
Member

I'm not aware of anything better than LinkedList for what you're doing here.

The only consideration I have is whether we actually need all of the synchronization as this operator will not interact with multiple threads and should be able to assume thread safety due to the Rx contract.

@benjchristensen
Copy link
Member

Merging as it is functional, but I'd be interested in a followup exploration of performance benefits of removing the synchronization and confirming whether my thoughts on the matter are correct.

benjchristensen added a commit that referenced this pull request Oct 9, 2013
Implemented the 'SkipLast' operator
@benjchristensen benjchristensen merged commit 4012519 into ReactiveX:master Oct 9, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
Implemented the 'SkipLast' operator
@zsxwing zsxwing deleted the skip-last branch February 13, 2014 13:16
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

4 participants