Hello! Thanks for sponsoring! If something is still necessary from my side regarding this issue, feel free to ask.
With best regards, Tagir Valeev. RR> Hi, RR> Looks good. RR> I would want to verify that it is defined such that if it later needs to be RR> abstracted up to ChronoLocalDate and apply to compatible Chronologies RR> and respective local dates such as JapaneseDate there is no conflict. RR> It should not be an issue since Period implements ChronoPeriod. RR> In the respective implementations, the estimation/computation of the RR> number of steps RR> would need to depend on the Chronology's definition of months. RR> I can sponsor this enhancement. RR> Thanks, Roger RR> On 1/20/16 10:15 AM, Stephen Colebourne wrote: >> I'm happy with the logic and specification of this proposal. I think it >> will be a useful addition. >> >> I'll let the Oracle team chime in to do a further review. >> >> thanks >> Stephen >> >> >> >> 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. >>> >>>
