Skip to content

Commit

Permalink
ICU-22669 Fix circular dependency in DateTimePatternGenerator::addICU…
Browse files Browse the repository at this point in the history 
…Patterns().
  • Loading branch information
@richgillam
richgillam committed Aug 27, 2024
1 parent 7c66c5c commit 967babd
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 46 deletions.
85 changes: 55 additions & 30 deletions icu4c/source/i18n/dtptngen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,38 +803,58 @@ DateTimePatternGenerator::staticGetBaseSkeleton(

void
DateTimePatternGenerator::addICUPatterns(const Locale& locale, UErrorCode& status) {
if (U_FAILURE(status)) { return; }
UnicodeString dfPattern;
UnicodeString conflictingString;
DateFormat* df;

// Load with ICU patterns
for (int32_t i=DateFormat::kFull; i<=DateFormat::kShort; i++) {
DateFormat::EStyle style = static_cast<DateFormat::EStyle>(i);
df = DateFormat::createDateInstance(style, locale);
SimpleDateFormat* sdf;
if (df != nullptr && (sdf = dynamic_cast<SimpleDateFormat*>(df)) != nullptr) {
sdf->toPattern(dfPattern);
addPattern(dfPattern, false, conflictingString, status);
}
// TODO Maybe we should return an error when the date format isn't simple.
delete df;
if (U_FAILURE(status)) { return; }
if (U_FAILURE(status)) {
return;
}

LocalUResourceBundlePointer rb(ures_open(nullptr, locale.getBaseName(), &status));
CharString calendarTypeToUse; // to be filled in with the type to use, if all goes well
getCalendarTypeToUse(locale, calendarTypeToUse, status);

df = DateFormat::createTimeInstance(style, locale);
if (df != nullptr && (sdf = dynamic_cast<SimpleDateFormat*>(df)) != nullptr) {
sdf->toPattern(dfPattern);
addPattern(dfPattern, false, conflictingString, status);
// HACK to get around the fact that the old SimpleDateFormat code (actually, Calendar::getCalendarTypeForLocale() )
// returns "gregorian" for ja_JP_TRADITIONAL instead of "japanese"
if (uprv_strcmp(locale.getBaseName(), "ja_JP_TRADITIONAL") == 0) {
calendarTypeToUse.clear().append("gregorian", status);
}

if (U_FAILURE(status)) {
return;
}

// TODO: C++ and Java are inconsistent (see #12568).
// C++ uses MEDIUM, but Java uses SHORT.
if ( i==DateFormat::kShort && !dfPattern.isEmpty() ) {
consumeShortTimePattern(dfPattern, status);
}
// TODO: See ICU-22867
CharString patternResourcePath;
patternResourcePath.append(DT_DateTimeCalendarTag, status)
.append('/', status)
.append(calendarTypeToUse, status)
.append('/', status)
.append(DT_DateTimePatternsTag, status);

LocalUResourceBundlePointer dateTimePatterns(ures_getByKeyWithFallback(rb.getAlias(), patternResourcePath.data(),
nullptr, &status));
if (ures_getType(dateTimePatterns.getAlias()) != URES_ARRAY || ures_getSize(dateTimePatterns.getAlias()) < 8) {
status = U_INVALID_FORMAT_ERROR;
return;
}

for (int32_t i = 0; U_SUCCESS(status) && i < DateFormat::kDateTime; i++) {
LocalUResourceBundlePointer patternRes(ures_getByIndex(dateTimePatterns.getAlias(), i, nullptr, &status));
UnicodeString pattern;
switch (ures_getType(patternRes.getAlias())) {
case URES_STRING:
pattern = ures_getUnicodeString(patternRes.getAlias(), &status);
break;
case URES_ARRAY:
pattern = ures_getUnicodeStringByIndex(patternRes.getAlias(), 0, &status);
break;
default:
status = U_INVALID_FORMAT_ERROR;
return;
}

if (U_SUCCESS(status)) {
UnicodeString conflictingPattern;
addPatternWithSkeleton(pattern, nullptr, false, conflictingPattern, status);
}
// TODO Maybe we should return an error when the date format isn't simple.
delete df;
if (U_FAILURE(status)) { return; }
}
}

Expand Down Expand Up @@ -905,7 +925,12 @@ DateTimePatternGenerator::getCalendarTypeToUse(const Locale& locale, CharString&
&localStatus);
localeWithCalendarKey[ULOC_LOCALE_IDENTIFIER_CAPACITY-1] = 0; // ensure null termination
// now get the calendar key value from that locale
destination = ulocimp_getKeywordValue(localeWithCalendarKey, "calendar", localStatus);
// (the call to ures_getFunctionalEquivalent() above might fail, and if it does, localeWithCalendarKey
// won't contain a `calendar` keyword. If this happens, the line below will stomp on `destination`,
// so we have to check the return code before overwriting `destination`.)
if (U_SUCCESS(localStatus)) {
destination = ulocimp_getKeywordValue(localeWithCalendarKey, "calendar", localStatus);
}
// If the input locale was invalid, don't fail with missing resource error, instead
// continue with default of Gregorian.
if (U_FAILURE(localStatus) && localStatus != U_MISSING_RESOURCE_ERROR) {
Expand Down
10 changes: 10 additions & 0 deletions icu4c/source/test/cintltst/udatpg_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,16 @@ static void TestOptions(void) {
{ "da", skel_Hmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_Hpmm },
{ "da", skel_HHmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_HHpmm },
{ "da", skel_hhmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_hhpmm_a },

// tests for ICU-22669
{ "zh_TW", u"jjm", UDATPG_MATCH_NO_OPTIONS, u"ah:mm" },
{ "zh_TW", u"jjm", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"ahh:mm" },
{ "zh_TW", u"jjms", UDATPG_MATCH_NO_OPTIONS, u"ah:mm:ss" },
{ "zh_TW", u"jjms", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"ahh:mm:ss" },
{ "zh_TW@hours=h23", u"jjm", UDATPG_MATCH_NO_OPTIONS, u"HH:mm" },
{ "zh_TW@hours=h23", u"jjm", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"HH:mm" }, // (without the fix, we get "HH:m" here)
{ "zh_TW@hours=h23", u"jjms", UDATPG_MATCH_NO_OPTIONS, u"HH:mm:ss" },
{ "zh_TW@hours=h23", u"jjms", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"HH:mm:ss" },
};

int count = UPRV_LENGTHOF(testData);
Expand Down
28 changes: 27 additions & 1 deletion icu4c/source/test/intltest/dtptngts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ void IntlTestDateTimePatternGeneratorAPI::runIndexedTest( int32_t index, UBool e
TESTCASE(11, test_jConsistencyOddLocales);
TESTCASE(12, testBestPattern);
TESTCASE(13, testDateTimePatterns);
TESTCASE(14, testRegionOverride);
TESTCASE(14, testISO8601);
TESTCASE(15, testRegionOverride);
default: name = ""; break;
}
}
Expand Down Expand Up @@ -1774,6 +1775,31 @@ void IntlTestDateTimePatternGeneratorAPI::testDateTimePatterns() {
}
}

// Test for ICU-21839: Make sure ISO8601 patterns/symbols are inherited from Gregorian
void IntlTestDateTimePatternGeneratorAPI::testISO8601() {
const char* localeIDs[] = {
"de-AT-u-ca-iso8601",
"de-CH-u-ca-iso8601",
};
const UChar* skeleton = u"jms";

for (int32_t i = 0; i < UPRV_LENGTHOF(localeIDs); i++) {
UErrorCode err = U_ZERO_ERROR;
LocalPointer<DateTimePatternGenerator> dtpg(DateTimePatternGenerator::createInstance(Locale::forLanguageTag(localeIDs[i], err), err));
UnicodeString pattern = dtpg->getBestPattern(skeleton, err);
if (pattern.indexOf(UnicodeString(u"├")) >= 0 || pattern.indexOf(UnicodeString(u"Minute")) >= 0) {
errln(UnicodeString("ERROR: locale ") + localeIDs[i] + UnicodeString(", skeleton ") + skeleton + UnicodeString(", bad pattern: ") + pattern);
}

LocalPointer<DateFormat> df(DateFormat::createTimeInstance(DateFormat::MEDIUM, Locale::forLanguageTag(localeIDs[i], err)));
UnicodeString format;
df->format(ucal_getNow(), format, err);
if (format.indexOf(UnicodeString(u"├")) >= 0 || format.indexOf(UnicodeString(u"Minute")) >= 0) {
errln(UnicodeString("ERROR: locale ") + localeIDs[i] + UnicodeString(", MEDIUM, bad format: ") + format);
}
}
}

void IntlTestDateTimePatternGeneratorAPI::testRegionOverride() {
const struct TestCase {
const char* locale;
Expand Down
1 change: 1 addition & 0 deletions icu4c/source/test/intltest/dtptngts.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class IntlTestDateTimePatternGeneratorAPI : public IntlTest {
void test_jConsistencyOddLocales();
void testBestPattern();
void testDateTimePatterns();
void testISO8601();
void testRegionOverride();

enum { kNumDateTimePatterns = 4 };
Expand Down
43 changes: 28 additions & 15 deletions icu4j/main/core/src/main/java/com/ibm/icu/text/DateTimePatternGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,7 @@
import java.util.TreeMap;
import java.util.TreeSet;

import com.ibm.icu.impl.ICUCache;
import com.ibm.icu.impl.ICUData;
import com.ibm.icu.impl.ICUResourceBundle;
import com.ibm.icu.impl.PatternTokenizer;
import com.ibm.icu.impl.SimpleCache;
import com.ibm.icu.impl.SimpleFormatterImpl;
import com.ibm.icu.impl.UResource;
import com.ibm.icu.impl.*;
import com.ibm.icu.util.Calendar;
import com.ibm.icu.util.Freezable;
import com.ibm.icu.util.ICUCloneNotSupportedException;
Expand Down Expand Up @@ -173,15 +167,34 @@ private void initData(ULocale uLocale, boolean skipStdPatterns) {

private void addICUPatterns(PatternInfo returnInfo, ULocale uLocale) {
// first load with the ICU patterns
for (int i = DateFormat.FULL; i <= DateFormat.SHORT; ++i) {
SimpleDateFormat df = (SimpleDateFormat) DateFormat.getDateInstance(i, uLocale);
addPattern(df.toPattern(), false, returnInfo);
df = (SimpleDateFormat) DateFormat.getTimeInstance(i, uLocale);
addPattern(df.toPattern(), false, returnInfo);

if (i == DateFormat.SHORT) {
consumeShortTimePattern(df.toPattern(), returnInfo);
ICUResourceBundle rb = (ICUResourceBundle) UResourceBundle.getBundleInstance(ICUData.ICU_BASE_NAME, uLocale);
String calendarTypeToUse = getCalendarTypeToUse(uLocale);
// TODO: this is a HACK to work around ICU-22668/CLDR-17892! Remove once they're fixed!
// (Without this hack, DateTimeGeneratorTest.testISO8601() fails. The corresponding test
// on the C++ side still _passes_, so I have NOT added this hack on the C++ side. --rtg 8/21/24)
if (calendarTypeToUse.equals("iso8601")) {
calendarTypeToUse = "gregorian";
}
// TODO: See ICU-22867
ICUResourceBundle dateTimePatterns = rb.getWithFallback("calendar/" + calendarTypeToUse + "/DateTimePatterns");
if (dateTimePatterns.getType() != UResourceBundle.ARRAY || dateTimePatterns.getSize() < 8) {
throw new MissingResourceException("Resource in wrong format", "ICUResourceBundle", "calendar/" + calendarTypeToUse + "/DateTimePatterns");
}
for (int i = 0; i < 8; i++) { // no constants available for the resource indexes
String pattern;
UResourceBundle patternRes = dateTimePatterns.get(i);

switch (patternRes.getType()) {
case UResourceBundle.STRING:
pattern = patternRes.getString();
break;
case UResourceBundle.ARRAY:
pattern = patternRes.getString(0);
break;
default:
throw new MissingResourceException("Resource in wrong format", "ICUResourceBundle", "calendar/" + calendarTypeToUse + "/DateTimePatterns");
}
addPattern(pattern, false, returnInfo);
}
}

Expand Down
10 changes: 10 additions & 0 deletions icu4j/main/core/src/test/java/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1417,6 +1417,16 @@ public void TestOptions() {
new TestOptionsItem( "zh@calendar=chinese", "ULLL", "U\u5E74MMM", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem( "zh@calendar=chinese", "yMMM", "rU\u5E74MMM", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem( "zh@calendar=chinese", "GUMMM", "rU\u5E74MMM", DateTimePatternGenerator.MATCH_NO_OPTIONS ),

// tests for ICU-22669
new TestOptionsItem("zh_TW", "jjm", "ah:mm", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem("zh_TW", "jjm", "ahh:mm", DateTimePatternGenerator.MATCH_ALL_FIELDS_LENGTH ),
new TestOptionsItem("zh_TW", "jjms", "ah:mm:ss", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem("zh_TW", "jjms", "ahh:mm:ss", DateTimePatternGenerator.MATCH_ALL_FIELDS_LENGTH ),
new TestOptionsItem("zh_TW@hours=h23", "jjm", "HH:mm", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem("zh_TW@hours=h23", "jjm", "HH:mm", DateTimePatternGenerator.MATCH_ALL_FIELDS_LENGTH ), // (without the fix, we get "HH:m" here)
new TestOptionsItem("zh_TW@hours=h23", "jjms", "HH:mm:ss", DateTimePatternGenerator.MATCH_NO_OPTIONS ),
new TestOptionsItem("zh_TW@hours=h23", "jjms", "HH:mm:ss", DateTimePatternGenerator.MATCH_ALL_FIELDS_LENGTH ),
};

for (int i = 0; i < testOptionsData.length; ++i) {
Expand Down

0 comments on commit 967babd

Please sign in to comment.