From: Rich Gillam <62772518+richgillam@users.noreply.github.com> Date: Fri, 6 Aug 2021 21:00:41 +0000 (-0700) Subject: ICU-13353 Fixed several problems preventing horizontal resource inheritance from... X-Git-Tag: cldr/2021-09-15~10 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c37ca411fcc5b6ee2135db7541bdd0a4c12dc021;p=icu ICU-13353 Fixed several problems preventing horizontal resource inheritance from working as intended, and added a Java version of a unit test I'd previously only added on the C++ side. --- diff --git a/icu4c/source/common/uresbund.cpp b/icu4c/source/common/uresbund.cpp index 119faf7f293..a9c6459418c 100644 --- a/icu4c/source/common/uresbund.cpp +++ b/icu4c/source/common/uresbund.cpp @@ -1910,10 +1910,83 @@ static Resource getTableItemByKeyPath(const ResourceData *pResData, Resource tab return resource; } -U_CAPI UResourceBundle* U_EXPORT2 -ures_getByKeyWithFallback(const UResourceBundle *resB, - const char* inKey, - UResourceBundle *fillIn, +static void createPath(const char* origResPath, + int32_t origResPathLen, + const char* resPath, + int32_t resPathLen, + const char* inKey, + CharString& path, + UErrorCode* status) { + // This is a utility function used by ures_getByKeyWithFallback() below. This function builds a path from + // resPath and inKey, returning the result in `path`. Originally, this function just cleared `path` and + // appended resPath and inKey to it, but that caused problems for horizontal inheritance. + // + // In normal cases, resPath is the same as origResPath, but if ures_getByKeyWithFallback() has followed an + // alias, resPath may be different from origResPath. Not only may the existing path elements be different, + // but resPath may also have MORE path elements than origResPath did. If it does, those additional path + // elements SUPERSEDE the corresponding elements of inKey. So this code counts the number of elements in + // resPath and origResPath and, for each path element in resPath that doesn't have a counterpart in origResPath, + // deletes a path element from the beginning of inKey. The remainder of inKey is then appended to + // resPath to form the result. (We're not using uprv_strchr() here because resPath and origResPath may + // not be zero-terminated.) + path.clear(); + const char* key = inKey; + if (resPathLen > 0) { + path.append(resPath, resPathLen, *status); + if (U_SUCCESS(*status)) { + const char* resPathLimit = resPath + resPathLen; + const char* origResPathLimit = origResPath + origResPathLen; + const char* resPathPtr = resPath; + const char* origResPathPtr = origResPath; + + // Remove from the beginning of resPath the number of segments that are contained in origResPath. + // If origResPath has MORE segments than resPath, this will leave resPath as the empty string. + while (origResPathPtr < origResPathLimit && resPathPtr < resPathLimit) { + while (origResPathPtr < origResPathLimit && *origResPathPtr != RES_PATH_SEPARATOR) { + ++origResPathPtr; + } + if (origResPathPtr < origResPathLimit && *origResPathPtr == RES_PATH_SEPARATOR) { + ++origResPathPtr; + } + while (resPathPtr < resPathLimit && *resPathPtr != RES_PATH_SEPARATOR) { + ++resPathPtr; + } + if (resPathPtr < resPathLimit && *resPathPtr == RES_PATH_SEPARATOR) { + ++resPathPtr; + } + } + + // New remove from the beginning of `key` the number of segments remaining in resPath. + // If resPath has more segments than `key` does, `key` will end up empty. + while (resPathPtr < resPathLimit && *key != '\0') { + while (resPathPtr < resPathLimit && *resPathPtr != RES_PATH_SEPARATOR) { + ++resPathPtr; + } + if (resPathPtr < resPathLimit && *resPathPtr == RES_PATH_SEPARATOR) { + ++resPathPtr; + } + while (*key != '\0' && *key != RES_PATH_SEPARATOR) { + ++key; + } + if (*key == RES_PATH_SEPARATOR) { + ++key; + } + } + } + // Finally, append what's left of `key` to `path`. What you end up with here is `resPath`, plus + // any pieces of `key` that aren't superseded by `resPath`. + // Or, to put it another way, calculate <#-segments-in-key> - (<#-segments-in-resPath> - <#-segments-in-origResPath>), + // and append that many segments from the end of `key` to `resPath` to produce the result. + path.append(key, *status); + } else { + path.append(inKey, *status); + } +} + +U_CAPI UResourceBundle* U_EXPORT2 +ures_getByKeyWithFallback(const UResourceBundle *resB, + const char* inKey, + UResourceBundle *fillIn, UErrorCode *status) { Resource res = RES_BOGUS, rootRes = RES_BOGUS; UResourceBundle *helper = NULL; @@ -1928,24 +2001,32 @@ ures_getByKeyWithFallback(const UResourceBundle *resB, int32_t type = RES_GET_TYPE(resB->fRes); if(URES_IS_TABLE(type)) { + const char* origResPath = resB->fResPath; + int32_t origResPathLen = resB->fResPathLen; res = getTableItemByKeyPath(&resB->getResData(), resB->fRes, inKey); const char* key = inKey; + bool didRootOnce = false; if(res == RES_BOGUS) { UResourceDataEntry *dataEntry = resB->fData; CharString path; char *myPath = NULL; const char* resPath = resB->fResPath; int32_t len = resB->fResPathLen; - while(res == RES_BOGUS && dataEntry->fParent != NULL) { /* Otherwise, we'll look in parents */ - dataEntry = dataEntry->fParent; + while(res == RES_BOGUS && (dataEntry->fParent != NULL || !didRootOnce)) { /* Otherwise, we'll look in parents */ + if (dataEntry->fParent != NULL) { + dataEntry = dataEntry->fParent; + } else { + // We can't just stop when we get to a bundle whose fParent is NULL. That'll work most of the time, + // but if the bundle that the caller passed to us was "root" (which happens in getAllItemsWithFallback(), + // this function will drop right out without doing anything if "root" doesn't contain the exact key path + // specified. In that case, we need one extra time through this loop to make sure we follow any + // applicable aliases at the root level. + didRootOnce = true; + } rootRes = dataEntry->fData.rootRes; if(dataEntry->fBogus == U_ZERO_ERROR) { - path.clear(); - if (len > 0) { - path.append(resPath, len, *status); - } - path.append(inKey, *status); + createPath(origResPath, origResPathLen, resPath, len, inKey, path, status); if (U_FAILURE(*status)) { ures_close(helper); return fillIn; @@ -1956,7 +2037,7 @@ ures_getByKeyWithFallback(const UResourceBundle *resB, res = res_findResource(&(dataEntry->fData), rootRes, &myPath, &key); if (RES_GET_TYPE(res) == URES_ALIAS && *myPath) { /* We hit an alias, but we didn't finish following the path. */ - helper = init_resb_result(dataEntry, res, NULL, -1, resB, helper, status); + helper = init_resb_result(dataEntry, res, NULL, -1, resB, helper, status); /*helper = init_resb_result(dataEntry, res, inKey, -1, resB, helper, status);*/ if(helper) { dataEntry = helper->fData; @@ -1982,7 +2063,26 @@ ures_getByKeyWithFallback(const UResourceBundle *resB, *status = U_USING_FALLBACK_WARNING; } - fillIn = init_resb_result(dataEntry, res, inKey, -1, resB, fillIn, status); + fillIn = init_resb_result(dataEntry, res, key, -1, resB, fillIn, status); + if (resPath != nullptr) { + createPath(origResPath, origResPathLen, resPath, len, inKey, path, status); + } else { + const char* separator = nullptr; + if (fillIn->fResPath != nullptr) { + separator = uprv_strchr(fillIn->fResPath, RES_PATH_SEPARATOR); + } + if (separator != nullptr && separator[1] != '\0') { + createPath(origResPath, origResPathLen, fillIn->fResPath, + static_cast(uprv_strlen(fillIn->fResPath)), inKey, path, status); + } else { + createPath(origResPath, origResPathLen, "", 0, inKey, path, status); + } + } + ures_freeResPath(fillIn); + ures_appendResPath(fillIn, path.data(), path.length(), status); + if(fillIn->fResPath[fillIn->fResPathLen-1] != RES_PATH_SEPARATOR) { + ures_appendResPath(fillIn, RES_PATH_SEPARATOR_S, 1, status); + } } else { *status = U_MISSING_RESOURCE_ERROR; } @@ -2001,8 +2101,7 @@ namespace { void getAllItemsWithFallback( const UResourceBundle *bundle, ResourceDataValue &value, - ResourceSink &sink, - UErrorCode &errorCode) { + ResourceSink &sink, UErrorCode &errorCode) { if (U_FAILURE(errorCode)) { return; } // We recursively enumerate child-first, // only storing parent items in the absence of child items. diff --git a/icu4c/source/common/uresimp.h b/icu4c/source/common/uresimp.h index 399abcfe868..4ac09bd8c4d 100644 --- a/icu4c/source/common/uresimp.h +++ b/icu4c/source/common/uresimp.h @@ -264,7 +264,6 @@ ures_getByKeyWithFallback(const UResourceBundle *resB, UResourceBundle *fillIn, UErrorCode *status); - /** * Get a String with multi-level fallback. Normally only the top level resources will * fallback to its parent. This performs fallback on subresources. For example, when a table diff --git a/icu4c/source/i18n/dtitvinf.cpp b/icu4c/source/i18n/dtitvinf.cpp index cd3ef6a57b0..6052894b586 100644 --- a/icu4c/source/i18n/dtitvinf.cpp +++ b/icu4c/source/i18n/dtitvinf.cpp @@ -50,7 +50,6 @@ U_NAMESPACE_BEGIN UOBJECT_DEFINE_RTTI_IMPLEMENTATION(DateIntervalInfo) static const char gCalendarTag[]="calendar"; -static const char gGenericTag[]="generic"; static const char gGregorianTag[]="gregorian"; static const char gIntervalDateTimePatternTag[]="intervalFormats"; static const char gFallbackPatternTag[]="fallback"; @@ -435,23 +434,6 @@ DateIntervalInfo::initializeData(const Locale& locale, UErrorCode& status) if ( U_SUCCESS(status) ) { resStr = ures_getStringByKeyWithFallback(itvDtPtnResource, gFallbackPatternTag, &resStrLen, &status); - if ( U_FAILURE(status) ) { - // Try to find "fallback" from "generic" to work around the bug in - // ures_getByKeyWithFallback - UErrorCode localStatus = U_ZERO_ERROR; - UResourceBundle *genericCalBundle = - ures_getByKeyWithFallback(calBundle, gGenericTag, nullptr, &localStatus); - UResourceBundle *genericItvDtPtnResource = - ures_getByKeyWithFallback( - genericCalBundle, gIntervalDateTimePatternTag, nullptr, &localStatus); - resStr = ures_getStringByKeyWithFallback( - genericItvDtPtnResource, gFallbackPatternTag, &resStrLen, &localStatus); - ures_close(genericItvDtPtnResource); - ures_close(genericCalBundle); - if ( U_SUCCESS(localStatus) ) { - status = U_USING_FALLBACK_WARNING;; - } - } } if ( U_SUCCESS(status) && (resStr != nullptr)) { diff --git a/icu4c/source/i18n/number_longnames.cpp b/icu4c/source/i18n/number_longnames.cpp index 08078c47473..25432135b35 100644 --- a/icu4c/source/i18n/number_longnames.cpp +++ b/icu4c/source/i18n/number_longnames.cpp @@ -259,20 +259,16 @@ class InflectedPluralSink : public ResourceSink { // See ResourceSink::put(). void put(const char *key, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) U_OVERRIDE { - ResourceTable pluralsTable = value.getTable(status); + int32_t pluralIndex = getIndex(key, status); if (U_FAILURE(status)) { return; } - for (int32_t i = 0; pluralsTable.getKeyAndValue(i, key, value); ++i) { - int32_t pluralIndex = getIndex(key, status); - if (U_FAILURE(status)) { return; } - if (!outArray[pluralIndex].isBogus()) { - // We already have a pattern - continue; - } - ResourceTable genderTable = value.getTable(status); - ResourceTable caseTable; // This instance has to outlive `value` - if (loadForPluralForm(genderTable, caseTable, value, status)) { - outArray[pluralIndex] = value.getUnicodeString(status); - } + if (!outArray[pluralIndex].isBogus()) { + // We already have a pattern + return; + } + ResourceTable genderTable = value.getTable(status); + ResourceTable caseTable; // This instance has to outlive `value` + if (loadForPluralForm(genderTable, caseTable, value, status)) { + outArray[pluralIndex] = value.getUnicodeString(status); } } @@ -370,18 +366,11 @@ void getInflectedMeasureData(StringPiece subKey, key.append(subKey, status); UErrorCode localStatus = status; - ures_getAllItemsWithFallback(unitsBundle.getAlias(), key.data(), sink, localStatus); + ures_getAllChildrenWithFallback(unitsBundle.getAlias(), key.data(), sink, localStatus); if (width == UNUM_UNIT_WIDTH_SHORT) { status = localStatus; return; } - - // TODO(ICU-13353): The fallback to short does not work in ICU4C. - // Manually fall back to short (this is done automatically in Java). - key.clear(); - key.append("unitsShort/", status); - key.append(subKey, status); - ures_getAllItemsWithFallback(unitsBundle.getAlias(), key.data(), sink, status); } class PluralTableSink : public ResourceSink { @@ -396,20 +385,16 @@ class PluralTableSink : public ResourceSink { } void put(const char *key, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) U_OVERRIDE { - ResourceTable pluralsTable = value.getTable(status); + if (uprv_strcmp(key, "case") == 0) { + return; + } + int32_t index = getIndex(key, status); if (U_FAILURE(status)) { return; } - for (int32_t i = 0; pluralsTable.getKeyAndValue(i, key, value); ++i) { - if (uprv_strcmp(key, "case") == 0) { - continue; - } - int32_t index = getIndex(key, status); - if (U_FAILURE(status)) { return; } - if (!outArray[index].isBogus()) { - continue; - } - outArray[index] = value.getUnicodeString(status); - if (U_FAILURE(status)) { return; } + if (!outArray[index].isBogus()) { + return; } + outArray[index] = value.getUnicodeString(status); + if (U_FAILURE(status)) { return; } } private: @@ -490,7 +475,7 @@ void getMeasureData(const Locale &locale, // getInflectedMeasureData after homogenizing data format? Find a unit // test case that demonstrates the incorrect fallback logic (via // regional variant of an inflected language?) - ures_getAllItemsWithFallback(unitsBundle.getAlias(), caseKey.data(), sink, localStatus); + ures_getAllChildrenWithFallback(unitsBundle.getAlias(), caseKey.data(), sink, localStatus); } // TODO(icu-units#138): our fallback logic is not spec-compliant: we @@ -499,20 +484,13 @@ void getMeasureData(const Locale &locale, // either get the spec changed, or add unit tests that warn us if // case="nominative" data differs from no-case data? UErrorCode localStatus = U_ZERO_ERROR; - ures_getAllItemsWithFallback(unitsBundle.getAlias(), key.data(), sink, localStatus); + ures_getAllChildrenWithFallback(unitsBundle.getAlias(), key.data(), sink, localStatus); if (width == UNUM_UNIT_WIDTH_SHORT) { if (U_FAILURE(localStatus)) { status = localStatus; } return; } - - // TODO(ICU-13353): The fallback to short does not work in ICU4C. - // Manually fall back to short (this is done automatically in Java). - key.clear(); - key.append("unitsShort", status); - key.append(subKey, status); - ures_getAllItemsWithFallback(unitsBundle.getAlias(), key.data(), sink, status); } // NOTE: outArray MUST have a length of at least ARRAY_LENGTH. @@ -523,7 +501,7 @@ void getCurrencyLongNameData(const Locale &locale, const CurrencyUnit ¤cy, PluralTableSink sink(outArray); LocalUResourceBundlePointer unitsBundle(ures_open(U_ICUDATA_CURR, locale.getName(), &status)); if (U_FAILURE(status)) { return; } - ures_getAllItemsWithFallback(unitsBundle.getAlias(), "CurrencyUnitPatterns", sink, status); + ures_getAllChildrenWithFallback(unitsBundle.getAlias(), "CurrencyUnitPatterns", sink, status); if (U_FAILURE(status)) { return; } for (int32_t i = 0; i < StandardPlural::Form::COUNT; i++) { UnicodeString &pattern = outArray[i]; diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index d5c539372c5..5117906b7e2 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -1232,7 +1232,7 @@ void NumberFormatterApiTest::unitArbitraryMeasureUnits() { .locale("en-ZA"); lnf.operator=(lnf); // self-assignment should be a no-op lnf.formatInt(1, status); - status.expectErrorAndReset(U_RESOURCE_TYPE_MISMATCH); + status.expectErrorAndReset(U_INTERNAL_PROGRAM_ERROR); assertFormatSingle( u"kibijoule-foot-per-cubic-gigafurlong-square-second unit-width-full-name", diff --git a/icu4c/source/test/intltest/restsnew.cpp b/icu4c/source/test/intltest/restsnew.cpp index 67b9f556326..a3e739a7e3e 100644 --- a/icu4c/source/test/intltest/restsnew.cpp +++ b/icu4c/source/test/intltest/restsnew.cpp @@ -1399,22 +1399,10 @@ void NewResourceBundleTest::TestFilter() { } } -/* - * The following test for ICU-20706 has infinite loops on certain inputs for - * locales and calendars. In order to unblock the build (ICU-21055), those - * specific values are temporarily removed. - * The issue of the infinite loops and its blocking dependencies were captured - * in ICU-21080. - */ - void NewResourceBundleTest::TestIntervalAliasFallbacks() { const char* locales[] = { - // Thee will not cause infinity loop "en", "ja", - - // These will cause infinity loop -#if 0 "fr_CA", "en_150", "es_419", @@ -1426,15 +1414,10 @@ void NewResourceBundleTest::TestIntervalAliasFallbacks() { "zh_Hant", "zh_Hant_TW", "zh_TW", -#endif }; const char* calendars[] = { - // These won't cause infinity loop "gregorian", "chinese", - - // These will cause infinity loop -#if 0 "islamic", "islamic-civil", "islamic-tbla", @@ -1443,7 +1426,6 @@ void NewResourceBundleTest::TestIntervalAliasFallbacks() { "islamic-rgsa", "japanese", "roc", -#endif }; for (int lidx = 0; lidx < UPRV_LENGTHOF(locales); lidx++) { @@ -1458,10 +1440,8 @@ void NewResourceBundleTest::TestIntervalAliasFallbacks() { key.append("calendar/", status); key.append(calendars[cidx], status); key.append("/intervalFormats/fallback", status); - if (! logKnownIssue("20400")) { - int32_t resStrLen = 0; - ures_getStringByKeyWithFallback(rb, key.data(), &resStrLen, &status); - } + int32_t resStrLen = 0; + ures_getStringByKeyWithFallback(rb, key.data(), &resStrLen, &status); if (U_FAILURE(status)) { errln("Cannot ures_getStringByKeyWithFallback('%s') on locale %s", key.data(), locales[lidx]); diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java index c1752c3919d..b12aadf0bf4 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java @@ -1854,4 +1854,30 @@ public class DateTimeGeneratorTest extends TestFmwk { } } + // Test for ICU-21202: Make sure DateTimePatternGenerator supplies an era field for year formats using the + // Buddhist and Japanese calendars for all English-speaking locales. + @Test + public void testEras() { + String[] localeIDs = { + "en_US@calendar=japanese", + "en_GB@calendar=japanese", + "en_150@calendar=japanese", + "en_001@calendar=japanese", + "en@calendar=japanese", + "en_US@calendar=buddhist", + "en_GB@calendar=buddhist", + "en_150@calendar=buddhist", + "en_001@calendar=buddhist", + "en@calendar=buddhist", + }; + + for (String localeID : localeIDs) { + DateTimePatternGenerator dtpg = DateTimePatternGenerator.getInstance(new Locale(localeID)); + String pattern = dtpg.getBestPattern("y"); + + if (pattern.indexOf('G') < 0) { + errln("missing era field for locale " + localeID); + } + } + } }