Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 07:00:23 GMT, Justin Lu wrote: >> Please review this PR which restricts sub-classing of the internal calendar >> system in sun.util.calendar to only the existing implementations. Drive by >> cleanup included. >> >> As the implementation is long-standing and complete with no intent for >> future sub-classing, the CalendarSystem should be made sealed. Modifiers >> adjusted accordingly (`JulianCalendar.Date` must now have package >> visibility). >> >> >> This system has the following structure, >> >> `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar` >> extended by >> (`Gregorian, JulianCalendar, LocalGregorianCalendar`) >> >> `CalendarDate` extended by `BaseCalendar.Date` extended by >> (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date, >> LocalGregorianCalendar.Date`) >> >> Additionally, CalendarUtils was made `final`, as it is a utility class >> composed of static util methods. > > Justin Lu has updated the pull request incrementally with two additional > commits since the last revision: > > - cleanup CalendarDate after revert > - Revert "Replace sprintf0d with Formatter" > >This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15803#pullrequestreview-1636151330
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 08:54:55 GMT, Andrey Turbanov wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - cleanup CalendarDate after revert
>> - Revert "Replace sprintf0d with Formatter"
>>
>>This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc.
>
> src/java.base/share/classes/sun/util/calendar/CalendarDate.java line 63:
>
>> 61: */
>> 62: public sealed abstract class CalendarDate implements Cloneable
>> 63: permits BaseCalendar.Date {
>
> Can we just merge `CalendarDate` and `BaseCalendar.Date` to be the one class?
> I think it will greatly simplify the code.
`BaseCalendar` is for Gregorian based calendars, so its `Date` class also
represents dates for those calendars, while `CalendarDate` is an abstract class
for all calendar implementations. So I don't think merging them is the right
direction.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1331943059
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
On Wed, 20 Sep 2023 07:00:23 GMT, Justin Lu wrote:
>> Please review this PR which restricts sub-classing of the internal calendar
>> system in sun.util.calendar to only the existing implementations. Drive by
>> cleanup included.
>>
>> As the implementation is long-standing and complete with no intent for
>> future sub-classing, the CalendarSystem should be made sealed. Modifiers
>> adjusted accordingly (`JulianCalendar.Date` must now have package
>> visibility).
>>
>>
>> This system has the following structure,
>>
>> `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar`
>> extended by
>> (`Gregorian, JulianCalendar, LocalGregorianCalendar`)
>>
>> `CalendarDate` extended by `BaseCalendar.Date` extended by
>> (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date,
>> LocalGregorianCalendar.Date`)
>>
>> Additionally, CalendarUtils was made `final`, as it is a utility class
>> composed of static util methods.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - cleanup CalendarDate after revert
> - Revert "Replace sprintf0d with Formatter"
>
>This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc.
src/java.base/share/classes/sun/util/calendar/CalendarDate.java line 63:
> 61: */
> 62: public sealed abstract class CalendarDate implements Cloneable
> 63: permits BaseCalendar.Date {
Can we just merge `CalendarDate` and `BaseCalendar.Date` to be the one class?
I think it will greatly simplify the code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1331283073
Re: RFR: JDK-8316435: sun.util.calendar.CalendarSystem subclassing should be restricted [v5]
> Please review this PR which restricts sub-classing of the internal calendar > system in sun.util.calendar to only the existing implementations. Drive by > cleanup included. > > As the implementation is long-standing and complete with no intent for future > sub-classing, the CalendarSystem should be made sealed. Modifiers adjusted > accordingly (`JulianCalendar.Date` must now have package visibility). > > > This system has the following structure, > > `CalendarSystem` extended by `AbstractCalendar` extended by `BaseCalendar` > extended by > (`Gregorian, JulianCalendar, LocalGregorianCalendar`) > > `CalendarDate` extended by `BaseCalendar.Date` extended by > (`Gregorian.Date, ImmutableGregorianDate, JulianCalendar.Date, > LocalGregorianCalendar.Date`) > > Additionally, CalendarUtils was made `final`, as it is a utility class > composed of static util methods. Justin Lu has updated the pull request incrementally with two additional commits since the last revision: - cleanup CalendarDate after revert - Revert "Replace sprintf0d with Formatter" This reverts commit 84a346aed2be262b717f82fbbc32a4ed0323bccc. - Changes: - all: https://git.openjdk.org/jdk/pull/15803/files - new: https://git.openjdk.org/jdk/pull/15803/files/790f7506..aaee9aa9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15803=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15803=03-04 Stats: 85 lines in 7 files changed: 45 ins; 5 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/15803.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15803/head:pull/15803 PR: https://git.openjdk.org/jdk/pull/15803
