Hello! Thank you for review! I did very small change in unit test comments to make them less confusing. Here's updated webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8146218/r3/
With best regards, Tagir Valeev. SC> I'm happy with the logic and specification of this proposal. I think it SC> will be a useful addition. SC> I'll let the Oracle team chime in to do a further review. SC> thanks SC> Stephen SC> On 16 January 2016 at 13:31, Tagir F. Valeev <amae...@gmail.com> wrote: >> Hello! >> >> Thanks for review! Here's the updated patch: >> http://cr.openjdk.java.net/~tvaleev/webrev/8146218/r2/ >> >> SC> The docs say that if the end date is before the start date, the >> SC> stream is empty. While I can see just about why LongStream.range() >> SC> works that way, I don't think this API should. The minimum is an >> SC> exception, but it would be easy to support negative in the >> SC> days-only case or the months-only case. The problem is where there >> SC> are both months/years and days and those should be exceptions. >> >> Now datesUntil(endExclusive) throws an IllegalArgumentException if end >> date is before start date. >> >> datesUntil(endExclusive, step) supports negative periods. It throws >> IllegalArgumentException if: >> - step is zero >> - step.toTotalMonths() and step.getDays() both non-zero and have >> opposite sign >> - step is negative and end date is after start date >> - step is positive and end date is before start date >> >> Otherwise it works correctly: you can use >> LocalDate.of(2016, 1, 1) >> .datesUntil(LocalDate.of(2015, 1, 1), Period.ofMonths(-1)); >> >> Steps like Period.of(-1, -1, -1) are also supported. >> >> SC> The single-arg implementation uses plusDays() with an >> SC> incrementing number. When the performance patch goes in, the >> SC> proposed streaming implementation will be optimal for small >> SC> streams but less optimal for large ones. The fastest way to loop >> SC> over a list of dates would be to manually generate them by >> SC> incrementing the day until it exceeds the length of month, and so >> SC> on. However, this would be serial. >> >> As I understand, plusDays performance patch is already pushed. >> >> It's possible to write custom Spliterator which would use >> previous.plusDays(1) in tryAdvance() and jump (via >> LocalDate.of(startEpochDay+n)) in trySplit() if parallel processing is >> requested. However this adds at least one additional class and more >> complexity. I guess, such optimization can be considered later as >> separate issue when API is stabilized. >> >> SC> As such, I think the best way to write this, taking account of >> SC> how plusDays() is implemented, is as follows: >> >> SC> LongStream.range(start.toEpochDay(), >> SC> end.toEpochDay()).mapToObj(LocalDate::ofEpochDay); >> >> Ok, now it's done this way. >> >> SC> In the period-based method, it would be best to capture the case >> SC> where totalMonths == 0 and days > 0 and forward to another method >> SC> that ignores months. That private method can share implementation >> SC> with the public single-arg method (passing in 1). >> >> This case still more complex than one-day case as it requires division >> and multiplication. Thus I'd keep these case separately. However I >> simplified "months = 0" path using ofEpochDay, now it should be >> faster. >> >> SC> The docs for the period-based method should specify the strategy >> SC> that produces the results (in abstract terms). This needs to cover >> SC> that the result is equivalent to always adding to the start period >> SC> a multiple of the period. >> >> I added some clarifications, please check. >> >> SC> Some nits: >> SC> I prefer to avoid @link in the first sentence. Just using 'stream' >> would be sufficient. >> >> Done. >> >> SC> The first sentence should be a summary. In this case it probably has a >> bit too much detail. >> >> Done. >> >> SC> The @return has 'values' on a new line when it could be on the same >> line. >> >> I set now line length = 100 characters in my IDE. Is it ok? >> >> SC> If statements need braces. >> >> Done. >> >> With best regards, >> Tagir Valeev. >> >>
