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

2.x: Add Observable.rangeLong & Flowable.rangeLong #4687

Merged
merged 5 commits into from
Oct 11, 2016
Merged

2.x: Add Observable.rangeLong & Flowable.rangeLong #4687

merged 5 commits into from
Oct 11, 2016

Conversation

vanniktech
Copy link
Collaborator

Will fix #4683

  • adds Observable.rangeLong()
  • adds Flowable.rangeLong()

both implementations are copied same for the unit tests.

What should we do about the range overflow check? We could use BigDecimal to check that.

Copy link
Member

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

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

Please also make sure the coverage of Flowable and Observable itself remains 100%

return just(start);
}

if (start + (count - 1) > Long.MAX_VALUE) {
Copy link
Member

Choose a reason for hiding this comment

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

That is always false. You should check if start + count - 1 flips to negative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know that's why I wrote What should we do about the range overflow check? We could use BigDecimal to check that.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can do Long.MAX_VALUE - count > start or check just if it overflows negative like @akarnokd said.

Copy link
Member

Choose a reason for hiding this comment

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

See intervalRange:

        long end = start + (count - 1);
        if (start > 0 && end < 0) {
            throw new IllegalArgumentException("Overflow! start + count is bigger than Long.MAX_VALUE");
        }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh cool didn't know that. Will change it thanks.

Observer<? super Long> actual = this.actual;
long e = end;
for (long i = index; i != e && get() == 0; i++) {
actual.onNext((long)i);
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary cast to long

return just(start);
}

if (start + (count - 1) > Long.MAX_VALUE) {
Copy link
Member

Choose a reason for hiding this comment

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

Same overflow check here as in Flowable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

@akarnokd akarnokd added this to the 2.0 RC5 milestone Oct 11, 2016
@codecov-io
Copy link

codecov-io commented Oct 11, 2016

Current coverage is 81.89% (diff: 54.08%)

Merging #4687 into 2.x will decrease coverage by 0.24%

@@                2.x      #4687   diff @@
==========================================
  Files           565        567     +2   
  Lines         37440      37599   +159   
  Methods           0          0          
  Messages          0          0          
  Branches       5746       5786    +40   
==========================================
+ Hits          30753      30791    +38   
- Misses         4612       4715   +103   
- Partials       2075       2093    +18   

Powered by Codecov. Last update 497f35f...8dc710d

@akarnokd akarnokd merged commit 7e89c1f into ReactiveX:2.x Oct 11, 2016
@vanniktech vanniktech deleted the long branch October 11, 2016 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.x: Observable.range(long start, long count)
4 participants