RogerRiggs
commented
11 years ago That seems like a worthwhile improvement.
Closed jodastephen closed 11 years ago
RogerRiggs
commented
11 years ago That seems like a worthwhile improvement.
jodastephen
commented
11 years ago Change wasn't too complicated and existing tests just passed.
# HG changeset patch
# User scolebourne
# Date 1372630583 -3600
# Node ID 7df31b1ac74bea0e21230f40284026ac0470b8e9
# Parent 4467c647ade992da1f3bcff09a37e4fd0be000b8
Alter TemporalField.resolve to directly alter map
Simpler, less error prone, approach for implementors
See #322
diff -r 4467c647ade9 -r 7df31b1ac74b src/share/classes/java/time/format/Parsed.java
--- a/src/share/classes/java/time/format/Parsed.java Sat Jun 29 10:31:14 2013 +0100
+++ b/src/share/classes/java/time/format/Parsed.java Sun Jun 30 23:16:23 2013 +0100
@@ -255,42 +255,34 @@
// if any other fields, handle them
// any lenient date resolution should return epoch-day
if (fieldValues.size() > 0) {
- boolean changed = false;
+ int changedCount = 0;
outer:
- while (true) {
+ while (changedCount < 50) {
for (Map.Entry<TemporalField, Long> entry : fieldValues.entrySet()) {
TemporalField targetField = entry.getKey();
- Map<TemporalField, Long> changes = targetField.resolve(this, entry.getValue(), resolverStyle);
- if (changes != null) {
- changed = true;
- resolveFieldsMakeChanges(targetField, changes);
- fieldValues.remove(targetField); // helps avoid infinite loops
+ ChronoLocalDate<?> resolvedDate = targetField.resolve(fieldValues, chrono, zone, resolverStyle);
+ if (resolvedDate != null) {
+ updateCheckConflict(resolvedDate);
+ changedCount++;
+ continue outer; // have to restart to avoid concurrent modification
+ } else if (fieldValues.containsKey(targetField) == false) {
+ changedCount++;
continue outer; // have to restart to avoid concurrent modification
}
}
break;
}
+ if (changedCount == 50) { // catch infinite loops
+ throw new DateTimeException("One of the parsed fields has an incorrectly implemented resolve method");
+ }
// if something changed then have to redo ChronoField resolve
- if (changed) {
+ if (changedCount > 0) {
resolveDateFields();
resolveTimeFields();
}
}
}
- private void resolveFieldsMakeChanges(TemporalField targetField, Map<TemporalField, Long> changes) {
- for (Map.Entry<TemporalField, Long> change : changes.entrySet()) {
- TemporalField changeField = change.getKey();
- Long changeValue = change.getValue();
- Objects.requireNonNull(changeField, "changeField");
- if (changeValue != null) {
- updateCheckConflict(targetField, changeField, changeValue);
- } else {
- fieldValues.remove(changeField);
- }
- }
- }
-
private void updateCheckConflict(TemporalField targetField, TemporalField changeField, Long changeValue) {
Long old = fieldValues.put(changeField, changeValue);
if (old != null && old.longValue() != changeValue.longValue()) {
diff -r 4467c647ade9 -r 7df31b1ac74b src/share/classes/java/time/temporal/IsoFields.java
--- a/src/share/classes/java/time/temporal/IsoFields.java Sat Jun 29 10:31:14 2013 +0100
+++ b/src/share/classes/java/time/temporal/IsoFields.java Sun Jun 30 23:16:23 2013 +0100
@@ -71,6 +71,8 @@
import java.time.Duration;
import java.time.LocalDate;
+import java.time.ZoneId;
+import java.time.chrono.ChronoLocalDate;
import java.time.chrono.Chronology;
import java.time.chrono.IsoChronology;
import java.time.format.ResolverStyle;
@@ -340,17 +342,21 @@
return (R) temporal.with(DAY_OF_YEAR, temporal.getLong(DAY_OF_YEAR) + (newValue - curValue));
}
@Override
- public Map<TemporalField, Long> resolve(TemporalAccessor temporal, long doq, ResolverStyle resolverStyle) {
- if ((temporal.isSupported(YEAR) && temporal.isSupported(QUARTER_OF_YEAR)) == false) {
+ public ChronoLocalDate<?> resolve(
+ Map<TemporalField, Long> fieldValues, Chronology chronology, ZoneId zone, ResolverStyle resolverStyle) {
+ Long yearLong = fieldValues.get(YEAR);
+ Long qoyLong = fieldValues.get(QUARTER_OF_YEAR);
+ if (yearLong == null || qoyLong == null) {
return null;
}
- int y = temporal.get(YEAR); // validated
+ int y = YEAR.checkValidIntValue(yearLong); // always validate
+ long doq = fieldValues.get(DAY_OF_QUARTER);
LocalDate date;
if (resolverStyle == ResolverStyle.LENIENT) {
- long qoy = temporal.getLong(QUARTER_OF_YEAR); // unvalidated
- date = LocalDate.of(y, 1, 1).plusMonths(Math.multiplyExact(Math.subtractExact(qoy, 1), 3));
+ date = LocalDate.of(y, 1, 1).plusMonths(Math.multiplyExact(Math.subtractExact(qoyLong, 1), 3));
+ doq = Math.subtractExact(doq, 1);
} else {
- int qoy = temporal.get(QUARTER_OF_YEAR); // validated
+ int qoy = QUARTER_OF_YEAR.range().checkValidIntValue(qoyLong, QUARTER_OF_YEAR); // validated
date = LocalDate.of(y, ((qoy - 1) * 3) + 1, 1);
if (doq < 1 || doq > 90) {
if (resolverStyle == ResolverStyle.STRICT) {
@@ -359,13 +365,12 @@
range().checkValidValue(doq, this); // allow 1-92 rolling into next quarter
}
}
+ doq--;
}
- long epochDay = Math.addExact(date.toEpochDay(), Math.subtractExact(doq, 1));
- Map<TemporalField, Long> result = new HashMap<>(4, 1.0f);
- result.put(EPOCH_DAY, epochDay);
- result.put(YEAR, null);
- result.put(QUARTER_OF_YEAR, null);
- return result;
+ fieldValues.remove(this);
+ fieldValues.remove(YEAR);
+ fieldValues.remove(QUARTER_OF_YEAR);
+ return date.plusDays(doq);
}
@Override
public String toString() {
@@ -458,14 +463,18 @@
return (R) temporal.plus(Math.subtractExact(newValue, getFrom(temporal)), WEEKS);
}
@Override
- public Map<TemporalField, Long> resolve(TemporalAccessor temporal, long wowby, ResolverStyle resolverStyle) {
- if ((temporal.isSupported(WEEK_BASED_YEAR) && temporal.isSupported(DAY_OF_WEEK)) == false) {
+ public ChronoLocalDate<?> resolve(
+ Map<TemporalField, Long> fieldValues, Chronology chronology, ZoneId zone, ResolverStyle resolverStyle) {
+ Long wbyLong = fieldValues.get(WEEK_BASED_YEAR);
+ Long dowLong = fieldValues.get(DAY_OF_WEEK);
+ if (wbyLong == null || dowLong == null) {
return null;
}
- int wby = temporal.get(WEEK_BASED_YEAR); // validated
+ int wby = WEEK_BASED_YEAR.range().checkValidIntValue(wbyLong, WEEK_BASED_YEAR); // always validate
+ long wowby = fieldValues.get(WEEK_OF_WEEK_BASED_YEAR);
LocalDate date = LocalDate.of(wby, 1, 4);
if (resolverStyle == ResolverStyle.LENIENT) {
- long dow = temporal.getLong(DAY_OF_WEEK); // unvalidated
+ long dow = dowLong; // unvalidated
if (dow > 7) {
date = date.plusWeeks((dow - 1) / 7);
dow = ((dow - 1) % 7) + 1;
@@ -475,7 +484,7 @@
}
date = date.plusWeeks(Math.subtractExact(wowby, 1)).with(DAY_OF_WEEK, dow);
} else {
- int dow = temporal.get(DAY_OF_WEEK); // validated
+ int dow = DAY_OF_WEEK.checkValidIntValue(dowLong); // validated
if (wowby < 1 || wowby > 52) {
if (resolverStyle == ResolverStyle.STRICT) {
getWeekRange(date).checkValidValue(wowby, this); // only allow exact range
@@ -485,11 +494,10 @@
}
date = date.plusWeeks(wowby - 1).with(DAY_OF_WEEK, dow);
}
- Map<TemporalField, Long> result = new HashMap<>(2, 1.0f);
- result.put(EPOCH_DAY, date.toEpochDay());
- result.put(WEEK_BASED_YEAR, null);
- result.put(DAY_OF_WEEK, null);
- return result;
+ fieldValues.remove(this);
+ fieldValues.remove(WEEK_BASED_YEAR);
+ fieldValues.remove(DAY_OF_WEEK);
+ return date;
}
@Override
public String toString() {
diff -r 4467c647ade9 -r 7df31b1ac74b src/share/classes/java/time/temporal/JulianFields.java
--- a/src/share/classes/java/time/temporal/JulianFields.java Sat Jun 29 10:31:14 2013 +0100
+++ b/src/share/classes/java/time/temporal/JulianFields.java Sun Jun 30 23:16:23 2013 +0100
@@ -66,6 +66,9 @@
import static java.time.temporal.ChronoUnit.FOREVER;
import java.time.DateTimeException;
+import java.time.ZoneId;
+import java.time.chrono.ChronoLocalDate;
+import java.time.chrono.Chronology;
import java.time.format.ResolverStyle;
import java.util.Collections;
import java.util.Map;
@@ -287,15 +290,14 @@
//-----------------------------------------------------------------------
@Override
- public Map<TemporalField, Long> resolve(TemporalAccessor temporal, long value, ResolverStyle resolverStyle) {
- long epochDay;
+ public ChronoLocalDate<?> resolve(
+ Map<TemporalField, Long> fieldValues, Chronology chronology, ZoneId zone, ResolverStyle resolverStyle) {
+ long value = fieldValues.remove(this);
if (resolverStyle == ResolverStyle.LENIENT) {
- epochDay = Math.subtractExact(value, offset);
- } else {
- range().checkValidValue(value, this);
- epochDay = value - offset;
+ return chronology.dateEpochDay(Math.subtractExact(value, offset));
}
- return Collections.<TemporalField, Long>singletonMap(EPOCH_DAY, epochDay);
+ range().checkValidValue(value, this);
+ return chronology.dateEpochDay(value - offset);
}
//-----------------------------------------------------------------------
diff -r 4467c647ade9 -r 7df31b1ac74b src/share/classes/java/time/temporal/TemporalField.java
--- a/src/share/classes/java/time/temporal/TemporalField.java Sat Jun 29 10:31:14 2013 +0100
+++ b/src/share/classes/java/time/temporal/TemporalField.java Sun Jun 30 23:16:23 2013 +0100
@@ -62,6 +62,9 @@
package java.time.temporal;
import java.time.DateTimeException;
+import java.time.ZoneId;
+import java.time.chrono.ChronoLocalDate;
+import java.time.chrono.Chronology;
import java.time.format.ResolverStyle;
import java.util.Locale;
import java.util.Map;
@@ -305,46 +308,68 @@
<R extends Temporal> R adjustInto(R temporal, long newValue);
/**
- * Resolves this field to provide a simpler alternative.
+ * Resolves this field to provide a simpler alternative or a date.
* <p>
* This method is invoked during the resolve phase of parsing.
* It is designed to allow application defined fields to be simplified into
- * more standard fields, such as those on {@code ChronoField}.
+ * more standard fields, such as those on {@code ChronoField}, or into a date.
* <p>
- * The method will only be invoked if the specified temporal supports this field.
- * The value of this field is provided.
+ * Applications should not normally invoke this method directly.
+ *
+ * @implSpec
+ * If an implementation represents a field that can be simplified, or
+ * combined with others, then this method must be implemented.
* <p>
- * The temporal must be queried using the methods of {@code TemporalAccessor},
- * not using {@code getFrom}, {@code isSupportedBy} or {@code rangeRefinedBy}.
- * Before querying any field, implementations must ensure it is supported, as
- * exceptions of this type would negatively affect the calculation of a parsed result.
+ * The specified map contains the current state of the parse.
+ * The map is mutable and must be mutated to resolve the field and
+ * any related fields. This method will only be invoked during parsing
+ * if the map contains this field, and implementations should therefore
+ * assume this field is present.
* <p>
- * If this field can resolve, it must return a map, if not it must return null.
- * The returned map contains the changes to be made to the temporal, expressed
- * as field-value pairs. If the value for a field is null, the field is to be
- * removed from the temporal. A null key must not be added to the result map.
+ * Resolving a field will consist of looking at the value of this field,
+ * and potentially other fields, and either updating the map with a
+ * simpler value, such as a {@code ChronoField}, or returning a
+ * complete {@code ChronoLocalDate}. If a resolve is successful,
+ * the code must remove all the fields that were resolved from the map,
+ * including this field.
* <p>
- * If the result is non-null, this field will be removed from the temporal.
- * This field should not be added to the result map.
+ * For example, the {@code IsoFields} class contains the quarter-of-year
+ * and day-of-quarter fields. The implementation of this method in that class
+ * resolves the two fields plus the {@link ChronoField#YEAR YEAR} into a
+ * complete {@code LocalDate}. The resolve method will remove all three
+ * fields from the map before returning the {@code LocalDate}.
* <p>
- * The {@link ResolverStyle} should be used by implementations to determine
- * how to perform the resolve.
+ * If resolution should be possible, but the data is invalid, the resolver
+ * style should be used to determine an appropriate level of leniency, which
+ * may require throwing a {@code DateTimeException} or {@code ArithmeticException}.
+ * If no resolution is possible, the resolve method must return null.
+ * <p>
+ * When resolving time fields, the map will be altered and null returned.
+ * When resolving date fields, the date is normally returned from the method,
+ * with the map altered to remove the resolved fields. However, it would also
+ * be acceptable for the date fields to be resolved into other {@code ChronoField}
+ * instances that can produce a date, such as {@code EPOCH_DAY}.
+ * <p>
+ * The zone is not normally required for resolution, but is provided for completeness.
* <p>
* The default implementation must return null.
*
- * @param temporal the temporal to resolve, not null
- * @param value the value of this field
+ * @param fieldValues the map of fields to values, which can be updated, not null
+ * @param chronology the effective chronology, not null
+ * @param zone the effective zone, not null
* @param resolverStyle the requested type of resolve, not null
- * @return a map of fields to update in the temporal, with a mapping to null
- * indicating a deletion. The whole map must be null if no resolving occurred
+ * @return the resolved date; null if resolving only changed the map,
+ * or no resolve occurred
+ * @throws ArithmeticException if numeric overflow occurs
* @throws DateTimeException if resolving results in an error. This must not be thrown
* by querying a field on the temporal without first checking if it is supported
- * @throws ArithmeticException if numeric overflow occurs
*/
- default Map<TemporalField, Long> resolve(
- TemporalAccessor temporal, long value, ResolverStyle resolverStyle) {
+ default ChronoLocalDate<?> resolve(
+ Map<TemporalField, Long> fieldValues, Chronology chronology,
+ ZoneId zone, ResolverStyle resolverStyle) {
return null;
}
+
/**
* Gets a descriptive name for the field.
* <p>
diff -r 4467c647ade9 -r 7df31b1ac74b src/share/classes/java/time/temporal/WeekFields.java
--- a/src/share/classes/java/time/temporal/WeekFields.java Sat Jun 29 10:31:14 2013 +0100
+++ b/src/share/classes/java/time/temporal/WeekFields.java Sun Jun 30 23:16:23 2013 +0100
@@ -77,6 +77,7 @@
import java.io.Serializable;
import java.time.DateTimeException;
import java.time.DayOfWeek;
+import java.time.ZoneId;
import java.time.chrono.ChronoLocalDate;
import java.time.chrono.Chronology;
import java.time.format.ResolverStyle;
@@ -756,6 +757,11 @@
return Math.floorMod(isoDow - sow, 7) + 1;
}
+ private int localizedDayOfWeek(int isoDow) {
+ int sow = weekDef.getFirstDayOfWeek().getValue();
+ return Math.floorMod(isoDow - sow, 7) + 1;
+ }
+
private long localizedWeekOfMonth(TemporalAccessor temporal) {
int dow = localizedDayOfWeek(temporal);
int dom = temporal.get(DAY_OF_MONTH);
@@ -885,7 +891,9 @@
}
@Override
- public Map<TemporalField, Long> resolve(TemporalAccessor temporal, long value, ResolverStyle resolverStyle) {
+ public ChronoLocalDate<?> resolve(
+ Map<TemporalField, Long> fieldValues, Chronology chronology, ZoneId zone, ResolverStyle resolverStyle) {
+ final long value = fieldValues.get(this);
final int newValue = Math.toIntExact(value); // broad limit makes overflow checking lighter
// first convert localized day-of-week to ISO day-of-week
// doing this first handles case where both ISO and localized were parsed and might mismatch
@@ -894,36 +902,38 @@
final int checkedValue = range.checkValidIntValue(value, this); // no leniency as too complex
final int startDow = weekDef.getFirstDayOfWeek().getValue();
long isoDow = Math.floorMod((startDow - 1) + (checkedValue - 1), 7) + 1;
- return Collections.<TemporalField, Long>singletonMap(DAY_OF_WEEK, isoDow);
+ fieldValues.remove(this);
+ fieldValues.put(DAY_OF_WEEK, isoDow);
+ return null;
}
// can only build date if ISO day-of-week is present
- if (temporal.isSupported(DAY_OF_WEEK) == false) {
+ if (fieldValues.containsKey(DAY_OF_WEEK) == false) {
return null;
}
- int dow = localizedDayOfWeek(temporal);
+ int isoDow = DAY_OF_WEEK.checkValidIntValue(fieldValues.get(DAY_OF_WEEK));
+ int dow = localizedDayOfWeek(isoDow);
// build date
- Chronology chrono = Chronology.from(temporal); // defaults to ISO
- if (temporal.isSupported(YEAR)) {
- int year = temporal.get(YEAR); // validate
- if (rangeUnit == MONTHS && temporal.isSupported(MONTH_OF_YEAR)) { // week-of-month
- long month = temporal.getLong(MONTH_OF_YEAR); // not validates yet
- return resolveWoM(chrono, year, month, newValue, dow, resolverStyle);
+ if (fieldValues.containsKey(YEAR)) {
+ int year = YEAR.checkValidIntValue(fieldValues.get(YEAR)); // validate
+ if (rangeUnit == MONTHS && fieldValues.containsKey(MONTH_OF_YEAR)) { // week-of-month
+ long month = fieldValues.get(MONTH_OF_YEAR); // not validated yet
+ return resolveWoM(fieldValues, chronology, year, month, newValue, dow, resolverStyle);
}
if (rangeUnit == YEARS) { // week-of-year
- return resolveWoY(chrono, year, newValue, dow, resolverStyle);
+ return resolveWoY(fieldValues, chronology, year, newValue, dow, resolverStyle);
}
} else if ((rangeUnit == WEEK_BASED_YEARS || rangeUnit == FOREVER) &&
- temporal.isSupported(weekDef.weekBasedYear) &&
- temporal.isSupported(weekDef.weekOfWeekBasedYear)) { // week-of-week-based-year and year-of-week-based-year
- return resolveWBY(chrono, temporal, dow, resolverStyle);
+ fieldValues.containsKey(weekDef.weekBasedYear) &&
+ fieldValues.containsKey(weekDef.weekOfWeekBasedYear)) { // week-of-week-based-year and year-of-week-based-year
+ return resolveWBY(fieldValues, chronology, dow, resolverStyle);
}
return null;
}
- private Map<TemporalField, Long> resolveWoM(
- Chronology chrono, int year, long month, long wom, int localDow, ResolverStyle resolverStyle) {
+ private ChronoLocalDate<?> resolveWoM(
+ Map<TemporalField, Long> fieldValues, Chronology chrono, int year, long month, long wom, int localDow, ResolverStyle resolverStyle) {
ChronoLocalDate<?> date;
if (resolverStyle == ResolverStyle.LENIENT) {
date = chrono.date(year, 1, 1).plus(Math.subtractExact(month, 1), MONTHS);
@@ -941,16 +951,15 @@
throw new DateTimeException("Strict mode rejected resolved date as it is in a different month");
}
}
- Map<TemporalField, Long> result = new HashMap<>(4, 1.0f);
- result.put(YEAR, null);
- result.put(MONTH_OF_YEAR, null);
- result.put(DAY_OF_WEEK, null);
- result.put(EPOCH_DAY, date.toEpochDay());
- return result;
+ fieldValues.remove(this);
+ fieldValues.remove(YEAR);
+ fieldValues.remove(MONTH_OF_YEAR);
+ fieldValues.remove(DAY_OF_WEEK);
+ return date;
}
- private Map<TemporalField, Long> resolveWoY(
- Chronology chrono, int year, long woy, int localDow, ResolverStyle resolverStyle) {
+ private ChronoLocalDate<?> resolveWoY(
+ Map<TemporalField, Long> fieldValues, Chronology chrono, int year, long woy, int localDow, ResolverStyle resolverStyle) {
ChronoLocalDate<?> date = chrono.date(year, 1, 1);
if (resolverStyle == ResolverStyle.LENIENT) {
long weeks = Math.subtractExact(woy, localizedWeekOfYear(date));
@@ -965,35 +974,35 @@
throw new DateTimeException("Strict mode rejected resolved date as it is in a different year");
}
}
- Map<TemporalField, Long> result = new HashMap<>(4, 1.0f);
- result.put(YEAR, null);
- result.put(DAY_OF_WEEK, null);
- result.put(EPOCH_DAY, date.toEpochDay());
- return result;
+ fieldValues.remove(this);
+ fieldValues.remove(YEAR);
+ fieldValues.remove(DAY_OF_WEEK);
+ return date;
}
- private Map<TemporalField, Long> resolveWBY(
- Chronology chrono, TemporalAccessor temporal, int localDow, ResolverStyle resolverStyle) {
- int yowby = temporal.get(weekDef.weekBasedYear);
+ private ChronoLocalDate<?> resolveWBY(
+ Map<TemporalField, Long> fieldValues, Chronology chrono, int localDow, ResolverStyle resolverStyle) {
+ int yowby = weekDef.weekBasedYear.range().checkValidIntValue(
+ fieldValues.get(weekDef.weekBasedYear), weekDef.weekBasedYear);
ChronoLocalDate<?> date;
if (resolverStyle == ResolverStyle.LENIENT) {
date = ofWeekBasedYear(chrono, yowby, 1, localDow);
- long wowby = temporal.getLong(weekDef.weekOfWeekBasedYear);
+ long wowby = fieldValues.get(weekDef.weekOfWeekBasedYear);
long weeks = Math.subtractExact(wowby, 1);
date = date.plus(weeks, WEEKS);
} else {
- int wowby = temporal.get(weekDef.weekOfWeekBasedYear); // validate
+ int wowby = weekDef.weekOfWeekBasedYear.range().checkValidIntValue(
+ fieldValues.get(weekDef.weekOfWeekBasedYear), weekDef.weekOfWeekBasedYear); // validate
date = ofWeekBasedYear(chrono, yowby, wowby, localDow);
if (resolverStyle == ResolverStyle.STRICT && localizedWeekBasedYear(date) != yowby) {
throw new DateTimeException("Strict mode rejected resolved date as it is in a different week-based-year");
}
}
- Map<TemporalField, Long> result = new HashMap<>(4, 1.0f);
- result.put(weekDef.weekBasedYear, null);
- result.put(weekDef.weekOfWeekBasedYear, null);
- result.put(DAY_OF_WEEK, null);
- result.put(EPOCH_DAY, date.toEpochDay());
- return result;
+ fieldValues.remove(this);
+ fieldValues.remove(weekDef.weekBasedYear);
+ fieldValues.remove(weekDef.weekOfWeekBasedYear);
+ fieldValues.remove(DAY_OF_WEEK);
+ return date;
}
//-----------------------------------------------------------------------looks fine
jodastephen
commented
11 years ago
The current
TemporalFieldresolve is based on theTemporalAccessor:This requires implementors to return a map object each time they are called, which will have negative performance. In particular, most current implementations actually want to return a full date object, but can't. Thus they create a date object, return just the epoch-day, and then the
Parsedclass has to recreate an equivalent date object. Very inefficient (and the code is too complex for effective hotspotting).Propose to change the method on
TemporalFieldto work like that onChronology, where it is passed a mutable map to work on, and can return a date object. This pattern would reduce the difficulty of implementors (as they'd only have one simpler pattern to learn).In addition, this change would help avoid some pathological cases where user extensible fields become hard to write and resolve depending on their exact implementation. While I don't have an example to hand, I remember finding it very difficult to reliably implement a
QuarterofYearclass.