Fine by me, thanks Stephen
On 22 December 2015 at 07:59, nadeesh tv <nadeesh...@oracle.com> wrote: > Hi all, > > Please see the updated webrev > http://cr.openjdk.java.net/~ntv/8143413/webrev.04/ > > Changes : Included the changes suggested by Stephen > > Thanks and Regards, > Nadeesh > > > On 12/22/2015 12:30 AM, Stephen Colebourne wrote: >> >> The comment for the new LocalDate.EPOCH field should use 1970-01-01, >> not 1970-1-1. However, the code should omit the zeroes: >> Replace: >> LocalDate.of(1970, 01, 01) >> with >> LocalDate.of(1970, 1, 1) >> >> The new method should follow the documentation of the similar method >> on ChronoLocalDateTime: >> >> * This combines this local date with the specified time and >> offset to calculate the >> * epoch-second value, which is the number of elapsed seconds from >> 1970-01-01T00:00:00Z. >> * Instants on the time-line after the epoch are positive, earlier >> are negative. >> >> The same applies to the new method on LocalTime: >> >> * This combines this local time with the specified date and >> offset to calculate the >> * epoch-second value, which is the number of elapsed seconds from >> 1970-01-01T00:00:00Z. >> * Instants on the time-line after the epoch are positive, earlier >> are negative. >> >> The same applies to the new method on OffsetTime: >> >> * This combines this offset time with the specified date to >> calculate the >> * epoch-second value, which is the number of elapsed seconds from >> 1970-01-01T00:00:00Z. >> * Instants on the time-line after the epoch are positive, earlier >> are negative. >> >> The test cases look good. >> >> thanks >> Stephen >> >> >> On 17 December 2015 at 18:13, nadeesh tv <nadeesh...@oracle.com> wrote: >>> >>> Hi all, >>> >>> Please see the updated webrev >>> http://cr.openjdk.java.net/~ntv/8143413/webrev.03/ >>> >>> Thanks and Regards, >>> Nadeesh >>> >>> On 12/4/2015 3:56 AM, Stephen Colebourne wrote: >>>> >>>> In the interests of harmony and getting something in, I'll accept that. >>>> >>>> I think LocalDate.EPOCH is probably better than LocalDate.EPOCHDAY >>>> >>>> Stephen >>>> >>>> >>>> >>>> On 3 December 2015 at 22:09, Roger Riggs<roger.ri...@oracle.com> wrote: >>>>> >>>>> Hi Sherman, >>>>> >>>>> It makes sense to me to provide the missing time/date as an explicit >>>>> parameter >>>>> for toEpochSeconds instead of assuming some constant. >>>>> >>>>> localDate.toEpochSeconds(LocalTime.MIDNIGHT, ZoneOffset.UTC); >>>>> localTime.toEpochSeconds(LocalDate.EPOCHDAY, ZoneOffset.UTC); >>>>> offsetTime.toEpochSeconds(LocalDate.EPOCHDAY); >>>>> >>>>> We'll have to add the LocalDate.EPOCHDAY constant. >>>>> >>>>> It makes it a bit heavier weight to use but can still be optimized and >>>>> won't >>>>> create garbage. >>>>> >>>>> Roger >>>>> >>>>> >>>>> >>>>> On 12/01/2015 12:33 PM, Xueming Shen wrote: >>>>>> >>>>>> On 12/1/15 6:36 AM, Stephen Colebourne wrote: >>>>>>> >>>>>>> As Roger says, these new methods are about performance as well as >>>>>>> conversion. >>>>>>> >>>>>>> While I fully acknowledge the time methods make an assumption, it is >>>>>>> not a crazy one, after all 1970-01-01 is just zero. >>>>>>> >>>>>>> Key I think is it allows: >>>>>>> long epochSecs = date.toEpochSeconds(offset) + >>>>>>> time.toEpochSeconds(offset); >>>>>>> to efficiently merge two objects without garbage. >>>>>> >>>>>> So it's not about j.t.LD/LT <-> j.u.Date, but instead of the clean >>>>>> approach >>>>>> >>>>>> LocalDate date = ... >>>>>> LocalDate time = ... >>>>>> ZoneOffset offset = ... >>>>>> >>>>>> ==> long spochSecs = LocalDateTime.of(date, >>>>>> time).toEpochSeconds(offset); >>>>>> >>>>>> we are adding APIs to provide a "fastpath" with the special assumption >>>>>> that the LocalDate "date" >>>>>> here is actually a "LocalDateTime" object ("date" + >>>>>> LocalTime.MIDNIGHT) >>>>>> and the LocalTime "time" >>>>>> again actually means a "LocalDateTime" (the "time" of 1970-01-01), to >>>>>> let >>>>>> the third party "libraries" >>>>>> to fool the basic date/time abstract in java.time package, simply to >>>>>> avoid creating the garbage >>>>>> middle man, localDateTime? it really does not sound right to me. >>>>>> >>>>>> First, if someone needs to mix/link LocalDate, LocalTime and Offset to >>>>>> epoch seconds in their >>>>>> libraries, they really SHOULD think hard about the conversion and make >>>>>> it >>>>>> right (it should not >>>>>> be encouraged to use these classes LocalDate, LocalTime and Offset >>>>>> without >>>>>> even understand >>>>>> what these classes are). But if we really have to provide such >>>>>> fastpath, >>>>>> personally I think it might >>>>>> be better either to provide these "utility/convenient" methods in a >>>>>> "utilities" class, or with an >>>>>> explicit date/time parameters (instead of the false assumption) for >>>>>> the >>>>>> missing date/time piece, >>>>>> such as >>>>>> >>>>>> localDate.toEpochSeconds(LocalTime.MIDNIGHT, offset); >>>>>> localTime.toEpochSeconds(LocalDate.EPOCHDAY, offset); >>>>>> >>>>>> Sherman >>>>>> >>>>>> >>>>>>> It also means that no-one has to think hard about the conversion, as >>>>>>> it is just there. It tends to be when people try to work this stuff >>>>>>> out for themselves that they get it wrong. >>>>>>> >>>>>>> Stephen >>>>>>> >>>>>>> >>>>>>> On 1 December 2015 at 14:21, Roger Riggs<roger.ri...@oracle.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> Hi Sherman, >>>>>>>> >>>>>>>> On 11/30/2015 6:09 PM, Xueming Shen wrote: >>>>>>>>> >>>>>>>>> On 11/30/2015 01:26 PM, Stephen Colebourne wrote: >>>>>>>>>> >>>>>>>>>> Converting LocalDate<-> java.util.Date is the question with the >>>>>>>>>> largest number of votes on SO: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> http://stackoverflow.com/questions/21242110/convert-java-util-date-to-java-time-localdate/21242111 >>>>>>>>>> The proposed method is designed to make the conversion easier. It >>>>>>>>>> also >>>>>>>>>> plugs an obvious gap in the API. >>>>>>>>>> >>>>>>>>>> While the LocalTime/OffsetTime methods are of lower importance, >>>>>>>>>> not >>>>>>>>>> having them would result in inconsistency between the various >>>>>>>>>> classes. >>>>>>>>>> We've already added factory methods to LocalTime for Java 9, these >>>>>>>>>> are >>>>>>>>>> just the other half of the picture. >>>>>>>>>> >>>>>>>>> I'm not sure I understand the idea of "the proposed method is >>>>>>>>> designed >>>>>>>>> to >>>>>>>>> make the conversion easier", as the SO is mainly about >>>>>>>>> j.u.Date->LocalDate, >>>>>>>>> not the other way around, from LocalDate -> j.u.Date. >>>>>>>> >>>>>>>> I think the issue is about *other* libraries that manipulate time >>>>>>>> via >>>>>>>> epochSeconds >>>>>>>> not about j.u.Date conversions. The concern was about performance >>>>>>>> and >>>>>>>> creating garbage along the way. >>>>>>>> >>>>>>>> Roger >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> As I said in the previous email, it might be "common" to use the >>>>>>>>> j.u.Date >>>>>>>>> to >>>>>>>>> abstract a "local date" and/or a "local time" (no other choice) >>>>>>>>> before >>>>>>>>> java.time, >>>>>>>>> and now there is the need to provide a migration path from those >>>>>>>>> "local >>>>>>>>> date/ >>>>>>>>> time" to the j.t.LocalDate/Time. But convert backward from the new >>>>>>>>> date/time >>>>>>>>> type to the "old" j.u.Date should not be encouraged (guess this is >>>>>>>>> also >>>>>>>>> the >>>>>>>>> consensus we agreed on back to jsr203). >>>>>>>>> >>>>>>>>> What are the "factory methods" you are referring to here? >>>>>>>>> JDK-8133079, >>>>>>>>> The >>>>>>>>> LocalDate/LocalTime.ofInstant()? >>>>>>>>> (btw, these two methods see missing the "since 1.9/9" tag) >>>>>>>>> >>>>>>>>> It seems to me that the ofInstant(Instant, ZondId) is from a >>>>>>>>> "super-set" >>>>>>>>> of >>>>>>>>> date/time to a "sub-set", without any assumption of "default >>>>>>>>> value", >>>>>>>>> it >>>>>>>>> is >>>>>>>>> similar to the conversion from zdt->ldt->ld/lt, and I can see the >>>>>>>>> "small" >>>>>>>>> improvement >>>>>>>>> >>>>>>>>> from| >>>>>>>>> Date input = new Date(); >>>>>>>>> LocalDatedate >>>>>>>>> =input.toInstant().atZone(ZoneId.systemDefault()).toLocalDate();| >>>>>>>>> >>>>>>>>> to >>>>>>>>> >>>>>>>>> |LocalDatedate >>>>>>>>> =LocalDate.ofInstant(input.toInstant(),ZoneId.systemDefault()));| >>>>>>>>> >>>>>>>>> The proposed pair toEpochSecond() however is doing the other way >>>>>>>>> around >>>>>>>>> and >>>>>>>>> with an unusual assumption of the missing date/time piece to a >>>>>>>>> "default >>>>>>>>> value" >>>>>>>>> (midnight, the epoch day). >>>>>>>>> >>>>>>>>> personally I think >>>>>>>>> >>>>>>>>> localDate.atTime(LocalTime.MIDNIGHT).toEpochSecond(ZoneOffset); >>>>>>>>> localTime.atDate(LocalDate.EPOCHDATE).toEpochSecond(ZoneOffset); >>>>>>>>> >>>>>>>>> is clean and good enough to help this backward conversion (with the >>>>>>>>> addition >>>>>>>>> of LocalDate.EPOCHDATE/DAY constant). Maybe, the vm can even help >>>>>>>>> to >>>>>>>>> remove that LocalDateTime middle man, with some arrangement. >>>>>>>>> >>>>>>>>> -Sherman >>>>>>>>> >>>>>>>>>> Note that these methods are specifically not referencing >>>>>>>>>> java.util.Date itself. Epoch seconds is the appropriate >>>>>>>>>> intermediate >>>>>>>>>> form here, and still widely used. >>>>>>>>>> >>>>>>>>>> Stephen >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 30 November 2015 at 19:36, Xueming >>>>>>>>>> Shen<xueming.s...@oracle.com> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> On 11/30/2015 10:37 AM, Stephen Colebourne wrote: >>>>>>>>>>>> >>>>>>>>>>>> This is based on user difficulties picked up via Stack Overflow. >>>>>>>>>>>> These >>>>>>>>>>>> methods aim to provide a simpler and faster approach, >>>>>>>>>>>> particularly >>>>>>>>>>>> for >>>>>>>>>>>> cases converting to/from java.util.Date. >>>>>>>>>>> >>>>>>>>>>> Can you be a little more specific on this one? We now have >>>>>>>>>>> Instance<=> >>>>>>>>>>> Date, >>>>>>>>>>> and considerably we might add LocalDateTime<=> Date with an >>>>>>>>>>> offset, >>>>>>>>>>> if >>>>>>>>>>> really >>>>>>>>>>> really desired (for performance? to save a Instant object as the >>>>>>>>>>> bridge?). >>>>>>>>>>> But I'm >>>>>>>>>>> a little confused about the connection among LocalDate/LocalTime, >>>>>>>>>>> epoch >>>>>>>>>>> seconds >>>>>>>>>>> and j.u.Date here. Are you saying someone wants to convert >>>>>>>>>>> >>>>>>>>>>> j.t.LocalDate -> epoch seconds -> j.u.Date >>>>>>>>>>> j.t.LocalTime -> epoch seconds -> j.u.Date >>>>>>>>>>> >>>>>>>>>>> and uses the converted j.u.Date to represent a local date (date >>>>>>>>>>> with >>>>>>>>>>> time >>>>>>>>>>> part to >>>>>>>>>>> be 0) and/or the local time (with year/month/day to be epoch >>>>>>>>>>> time) >>>>>>>>>>> in >>>>>>>>>>> the >>>>>>>>>>> "old" >>>>>>>>>>> system which only has j.u.Date, and has to use the j.u.Date to >>>>>>>>>>> abstract >>>>>>>>>>> the >>>>>>>>>>> "local >>>>>>>>>>> date" and "local time"? >>>>>>>>>>> >>>>>>>>>>> I think we agreed back to JSR310 that we don't try to add such >>>>>>>>>>> kind >>>>>>>>>>> of >>>>>>>>>>> "bridge/ >>>>>>>>>>> convenient/utility" methods into the new java.time package, but >>>>>>>>>>> only >>>>>>>>>>> in >>>>>>>>>>> the >>>>>>>>>>> old date/calendar classes, if really needed. So if these methods >>>>>>>>>>> are >>>>>>>>>>> only to >>>>>>>>>>> help >>>>>>>>>>> migrate/bridge between the "old" and "new" calendar systems, the >>>>>>>>>>> java.time >>>>>>>>>>> might not be the best place for them? >>>>>>>>>>> >>>>>>>>>>>> For the time cases, the convention of 1970-01-01 is natural and >>>>>>>>>>>> commonly used in many codebases. >>>>>>>>>>>> >>>>>>>>>>> I'm not sure about that, especially the "natural" part. It might >>>>>>>>>>> be >>>>>>>>>>> "common" >>>>>>>>>>> in >>>>>>>>>>> the old days if you only have j.u.Date", and might be forced to >>>>>>>>>>> use >>>>>>>>>>> 1970-01-01 >>>>>>>>>>> to fill in the "date" part even when you are really only >>>>>>>>>>> interested >>>>>>>>>>> in >>>>>>>>>>> "time" part >>>>>>>>>>> of it in your app. One of the advantage of java.time.LDT/LD/LT is >>>>>>>>>>> now >>>>>>>>>>> we >>>>>>>>>>> have >>>>>>>>>>> separate abstract for these different need, I don't see the >>>>>>>>>>> common >>>>>>>>>>> need >>>>>>>>>>> of >>>>>>>>>>> having a LocalTime only meas the "local time" of 1970-01-01, or I >>>>>>>>>>> misunderstood >>>>>>>>>>> something here. >>>>>>>>>>> >>>>>>>>>>> -Sherman >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Stephen >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 30 November 2015 at 18:15, Xueming >>>>>>>>>>>> Shen<xueming.s...@oracle.com> >>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> While it is kinda understandable to have >>>>>>>>>>>>> LocalDate.toEpochSecond(...) >>>>>>>>>>>>> to get the epoch seconds since 1970.1.1, with the assumption of >>>>>>>>>>>>> the >>>>>>>>>>>>> time is at the midnight of that day. It is strange to have the >>>>>>>>>>>>> Local/OffsetTime >>>>>>>>>>>>> to have two public methods with the assumption of the "date" is >>>>>>>>>>>>> at >>>>>>>>>>>>> epoch >>>>>>>>>>>>> year/month/day. What's the use case/scenario for these two >>>>>>>>>>>>> proposed >>>>>>>>>>>>> methods? >>>>>>>>>>>>> >>>>>>>>>>>>> -Sherman >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 11/30/2015 06:36 AM, Stephen Colebourne wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> The method Javadoc (specs) for each of the three new methods >>>>>>>>>>>>>> needs >>>>>>>>>>>>>> to >>>>>>>>>>>>>> be enhanced. >>>>>>>>>>>>>> >>>>>>>>>>>>>> For the date ones it needs to say >>>>>>>>>>>>>> "The resulting date will have a time component of midnight at >>>>>>>>>>>>>> the >>>>>>>>>>>>>> start of the day." >>>>>>>>>>>>>> >>>>>>>>>>>>>> For the time ones it needs to say >>>>>>>>>>>>>> "The resulting time will be on 1970-01-01." >>>>>>>>>>>>>> >>>>>>>>>>>>>> Some of the line wrapping in the tests looks like it is not >>>>>>>>>>>>>> indented >>>>>>>>>>>>>> correctly. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Otherwise looks fine, >>>>>>>>>>>>>> thanks >>>>>>>>>>>>>> Stephen >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 30 November 2015 at 11:50, nadeesh >>>>>>>>>>>>>> tv<nadeesh...@oracle.com> >>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Please review a fix for >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Bug Id -https://bugs.openjdk.java.net/browse/JDK-8143413 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Issue - add toEpochSecond methods for efficient access >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> webrev -http://cr.openjdk.java.net/~ntv/8143413/webrev.01/ >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> -- Thanks and Regards, >>>>>>>>>>>>>>> Nadeesh TV >>>>>>>>>>>> >>>>>>>>>>>> <div id="DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2"><table >>>>>>>>>>>> style="border-top: 1px solid #aaabb6; margin-top: 10px;"> >>>>>>>>>>>> <tr> >>>>>>>>>>>> <td style="width: 105px; padding-top: >>>>>>>>>>>> 15px;"> >>>>>>>>>>>> <a >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> href="https://www.avast.com/?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail"; >>>>>>>>>>>> target="_blank"><img >>>>>>>>>>>> src="https://ipmcdn.avast.com/images/logo-avast-v1.png"; >>>>>>>>>>>> style="width: >>>>>>>>>>>> 90px; height:33px;"/></a> >>>>>>>>>>>> </td> >>>>>>>>>>>> <td style="width: 470px; padding-top: 20px; >>>>>>>>>>>> color: >>>>>>>>>>>> #41424e; >>>>>>>>>>>> font-size: 13px; font-family: Arial, Helvetica, sans-serif; >>>>>>>>>>>> line-height: 18px;">This email has been sent from a virus-free >>>>>>>>>>>> computer protected by Avast.<br /><a >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> href="https://www.avast.com/?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail"; >>>>>>>>>>>> target="_blank" style="color: #4453ea;">www.avast.com</a> >>>>>>>>>>>> </td> >>>>>>>>>>>> </tr> >>>>>>>>>>>> </table><a href="#DDB4FAA8-2DD7-40BB-A1B8-4E2AA1F9FDF2" >>>>>>>>>>>> width="1" >>>>>>>>>>>> height="1"></a></div> >>> >>> >>> -- >>> Thanks and Regards, >>> Nadeesh TV >>> > > -- > Thanks and Regards, > Nadeesh TV >
![https://ipmcdn.avast.com/images/logo-avast-v1.png"](https://ipmcdn.avast.com/images/logo-avast-v1.png"){kind=link}