On Mon, 18 Sep 2023 22:42:09 GMT, Justin Lu <j...@openjdk.org> wrote:
> Please review this PR which restricts sub-classing of the internal calendar
> system in sun.util.calendar to only the existing implementations.
>
> 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.
Looks good overall.
src/java.base/share/classes/sun/util/calendar/CalendarUtils.java line 30:
> 28: public final class CalendarUtils {
> 29:
> 30: private CalendarUtils() {}
It may be obvious, but adding a comment would be helpful, i.e., no instance may
be created.
src/java.base/share/classes/sun/util/calendar/CalendarUtils.java line 132:
> 130: * Mimics sprintf(buf, "%0*d", decaimal, width).
> 131: */
> 132: public static StringBuilder sprintf0d(StringBuilder sb, int value,
> int width) {
Can this (and the following overload) method be eliminated? No need to mimic
`sprintf` here, but using `String.format`/`formatted` should suffice?
src/java.base/share/classes/sun/util/calendar/Gregorian.java line 40:
> 38:
> 39: static final class Date extends BaseCalendar.Date {
> 40: Date() {
I think removing `protected` from the `final` class methods is fine. Adding
`@Override` may be helpful.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15803#pullrequestreview-1633738951
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1330414349
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1330430127
PR Review Comment: https://git.openjdk.org/jdk/pull/15803#discussion_r1330418996
