From 397cbb9530b613210ce8a486f8a94d058eec8c41 Mon Sep 17 00:00:00 2001 From: Victor Chang Date: Fri, 21 Jan 2022 14:52:55 +0000 Subject: [PATCH] ICU-21894 A modern version of ucol_safeClone and ucnv_safeClone API --- icu4c/source/common/ubrk.cpp | 12 +-- icu4c/source/common/ucnv.cpp | 11 ++- icu4c/source/common/unicode/ubrk.h | 3 +- icu4c/source/common/unicode/ucnv.h | 24 +++-- icu4c/source/i18n/ucol.cpp | 8 +- icu4c/source/i18n/unicode/ucol.h | 28 ++++-- icu4c/source/test/cintltst/capitst.c | 119 +++++++++++++++++++++++ icu4c/source/test/cintltst/capitst.h | 5 + icu4c/source/test/cintltst/ccapitst.c | 134 ++++++++++++++++++++++++++ 9 files changed, 316 insertions(+), 28 deletions(-) diff --git a/icu4c/source/common/ubrk.cpp b/icu4c/source/common/ubrk.cpp index bb5bdd1b501..f4e064961f3 100644 --- a/icu4c/source/common/ubrk.cpp +++ b/icu4c/source/common/ubrk.cpp @@ -168,7 +168,7 @@ ubrk_safeClone( BreakIterator *newBI = ((BreakIterator *)bi)->clone(); if (newBI == NULL) { *status = U_MEMORY_ALLOCATION_ERROR; - } else { + } else if (pBufferSize != NULL) { *status = U_SAFECLONE_ALLOCATED_WARNING; } return (UBreakIterator *)newBI; @@ -176,15 +176,7 @@ ubrk_safeClone( U_CAPI UBreakIterator * U_EXPORT2 ubrk_clone(const UBreakIterator *bi, UErrorCode *status) { - if (U_FAILURE(*status)) { - return nullptr; - } - BreakIterator *newBI = ((BreakIterator *)bi)->clone(); - if (newBI == nullptr) { - *status = U_MEMORY_ALLOCATION_ERROR; - return nullptr; - } - return (UBreakIterator *)newBI; + return ubrk_safeClone(bi, nullptr, nullptr, status); } diff --git a/icu4c/source/common/ucnv.cpp b/icu4c/source/common/ucnv.cpp index 5dcf35e0438..019bcb6a79c 100644 --- a/icu4c/source/common/ucnv.cpp +++ b/icu4c/source/common/ucnv.cpp @@ -252,7 +252,10 @@ ucnv_safeClone(const UConverter* cnv, void *stackBuffer, int32_t *pBufferSize, U UTRACE_EXIT_STATUS(*status); return NULL; } - *status = U_SAFECLONE_ALLOCATED_WARNING; + // If pBufferSize was NULL as the input, pBufferSize is set to &stackBufferSize in this function. + if (pBufferSize != &stackBufferSize) { + *status = U_SAFECLONE_ALLOCATED_WARNING; + } /* record the fact that memory was allocated */ *pBufferSize = bufferSizeNeeded; @@ -317,7 +320,11 @@ ucnv_safeClone(const UConverter* cnv, void *stackBuffer, int32_t *pBufferSize, U return localConverter; } - +U_CAPI UConverter* U_EXPORT2 +ucnv_clone(const UConverter* cnv, UErrorCode *status) +{ + return ucnv_safeClone(cnv, nullptr, nullptr, status); +} /*Decreases the reference counter in the shared immutable section of the object *and frees the mutable part*/ diff --git a/icu4c/source/common/unicode/ubrk.h b/icu4c/source/common/unicode/ubrk.h index c603f7c13f3..d6f0d258174 100644 --- a/icu4c/source/common/unicode/ubrk.h +++ b/icu4c/source/common/unicode/ubrk.h @@ -312,7 +312,8 @@ ubrk_openBinaryRules(const uint8_t *binaryRules, int32_t rulesLength, * If *pBufferSize is not enough for a stack-based safe clone, * new memory will be allocated. * @param status to indicate whether the operation went on smoothly or there were errors - * An informational status value, U_SAFECLONE_ALLOCATED_ERROR, is used if any allocations were necessary. + * An informational status value, U_SAFECLONE_ALLOCATED_ERROR, is used + * if pBufferSize != NULL and any allocations were necessary * @return pointer to the new clone * @deprecated ICU 69 Use ubrk_clone() instead. */ diff --git a/icu4c/source/common/unicode/ucnv.h b/icu4c/source/common/unicode/ucnv.h index 2687c984d43..9a7733155c6 100644 --- a/icu4c/source/common/unicode/ucnv.h +++ b/icu4c/source/common/unicode/ucnv.h @@ -477,7 +477,7 @@ ucnv_openCCSID(int32_t codepage, * *

The name will NOT be looked up in the alias mechanism, nor will the converter be * stored in the converter cache or the alias table. The only way to open further converters - * is call this function multiple times, or use the ucnv_safeClone() function to clone a + * is call this function multiple times, or use the ucnv_clone() function to clone a * 'primary' converter.

* *

A future version of ICU may add alias table lookups and/or caching @@ -493,13 +493,27 @@ ucnv_openCCSID(int32_t codepage, * @return the created Unicode converter object, or NULL if an error occurred * @see udata_open * @see ucnv_open - * @see ucnv_safeClone + * @see ucnv_clone * @see ucnv_close * @stable ICU 2.2 */ U_CAPI UConverter* U_EXPORT2 ucnv_openPackage(const char *packageName, const char *converterName, UErrorCode *err); +/** + * Thread safe converter cloning operation. + * + * You must ucnv_close() the clone. + * + * @param cnv converter to be cloned + * @param status to indicate whether the operation went on smoothly or there were errors + * @return pointer to the new clone + * @stable ICU 71 + */ +U_CAPI UConverter* U_EXPORT2 ucnv_clone(const UConverter *cnv, UErrorCode *status); + +#ifndef U_HIDE_DEPRECATED_API + /** * Thread safe converter cloning operation. * For most efficient operation, pass in a stackBuffer (and a *pBufferSize) @@ -532,12 +546,12 @@ ucnv_openPackage(const char *packageName, const char *converterName, UErrorCode * pointer to size of allocated space. * @param status to indicate whether the operation went on smoothly or there were errors * An informational status value, U_SAFECLONE_ALLOCATED_WARNING, - * is used if any allocations were necessary. + * is used if pBufferSize != NULL and any allocations were necessary * However, it is better to check if *pBufferSize grew for checking for * allocations because warning codes can be overridden by subsequent * function calls. * @return pointer to the new clone - * @stable ICU 2.0 + * @deprecated ICU 71 Use ucnv_clone() instead. */ U_CAPI UConverter * U_EXPORT2 ucnv_safeClone(const UConverter *cnv, @@ -545,8 +559,6 @@ ucnv_safeClone(const UConverter *cnv, int32_t *pBufferSize, UErrorCode *status); -#ifndef U_HIDE_DEPRECATED_API - /** * \def U_CNV_SAFECLONE_BUFFERSIZE * Definition of a buffer size that is designed to be large enough for diff --git a/icu4c/source/i18n/ucol.cpp b/icu4c/source/i18n/ucol.cpp index f59333ede3c..8e1df8d5577 100644 --- a/icu4c/source/i18n/ucol.cpp +++ b/icu4c/source/i18n/ucol.cpp @@ -96,12 +96,18 @@ ucol_safeClone(const UCollator *coll, void * /*stackBuffer*/, int32_t * pBufferS if (newColl == NULL) { *status = U_MEMORY_ALLOCATION_ERROR; return nullptr; - } else { + } else if (pBufferSize != NULL) { *status = U_SAFECLONE_ALLOCATED_WARNING; } return newColl->toUCollator(); } +U_CAPI UCollator* U_EXPORT2 +ucol_clone(const UCollator *coll, UErrorCode *status) +{ + return ucol_safeClone(coll, nullptr, nullptr, status); +} + U_CAPI void U_EXPORT2 ucol_close(UCollator *coll) { diff --git a/icu4c/source/i18n/unicode/ucol.h b/icu4c/source/i18n/unicode/ucol.h index 6d22eb6069e..a17804fddfe 100644 --- a/icu4c/source/i18n/unicode/ucol.h +++ b/icu4c/source/i18n/unicode/ucol.h @@ -397,7 +397,7 @@ typedef enum { * @param status A pointer to a UErrorCode to receive any errors * @return A pointer to a UCollator, or 0 if an error occurred. * @see ucol_openRules - * @see ucol_safeClone + * @see ucol_clone * @see ucol_close * @stable ICU 2.0 */ @@ -425,7 +425,7 @@ ucol_open(const char *loc, UErrorCode *status); * @return A pointer to a UCollator. It is not guaranteed that NULL be returned in case * of error - please use status argument to check for errors. * @see ucol_open - * @see ucol_safeClone + * @see ucol_clone * @see ucol_close * @stable ICU 2.0 */ @@ -521,7 +521,7 @@ ucol_getContractionsAndExpansions( const UCollator *coll, * @param coll The UCollator to close. * @see ucol_open * @see ucol_openRules - * @see ucol_safeClone + * @see ucol_clone * @stable ICU 2.0 */ U_CAPI void U_EXPORT2 @@ -985,7 +985,6 @@ ucol_getShortDefinitionString(const UCollator *coll, * * @deprecated ICU 54 */ - U_DEPRECATED int32_t U_EXPORT2 ucol_normalizeShortDefinitionString(const char *source, char *destination, @@ -1310,6 +1309,20 @@ U_DEPRECATED void U_EXPORT2 ucol_restoreVariableTop(UCollator *coll, const uint32_t varTop, UErrorCode *status); #endif /* U_HIDE_DEPRECATED_API */ +/** + * Thread safe cloning operation. The result is a clone of a given collator. + * @param coll collator to be cloned + * @param status to indicate whether the operation went on smoothly or there were errors + * @return pointer to the new clone + * @see ucol_open + * @see ucol_openRules + * @see ucol_close + * @stable ICU 71 + */ +U_CAPI UCollator* U_EXPORT2 ucol_clone(const UCollator *coll, UErrorCode *status); + +#ifndef U_HIDE_DEPRECATED_API + /** * Thread safe cloning operation. The result is a clone of a given collator. * @param coll collator to be cloned @@ -1325,13 +1338,13 @@ ucol_restoreVariableTop(UCollator *coll, const uint32_t varTop, UErrorCode *stat * If *pBufferSize is not enough for a stack-based safe clone, * new memory will be allocated. * @param status to indicate whether the operation went on smoothly or there were errors - * An informational status value, U_SAFECLONE_ALLOCATED_ERROR, is used if any - * allocations were necessary. + * An informational status value, U_SAFECLONE_ALLOCATED_ERROR, is used + * if pBufferSize != NULL and any allocations were necessary * @return pointer to the new clone * @see ucol_open * @see ucol_openRules * @see ucol_close - * @stable ICU 2.0 + * @deprecated ICU 71 Use ucol_clone() instead. */ U_CAPI UCollator* U_EXPORT2 ucol_safeClone(const UCollator *coll, @@ -1339,7 +1352,6 @@ ucol_safeClone(const UCollator *coll, int32_t *pBufferSize, UErrorCode *status); -#ifndef U_HIDE_DEPRECATED_API /** default memory size for the new clone. * @deprecated ICU 52. Do not rely on ucol_safeClone() cloning into any provided buffer. diff --git a/icu4c/source/test/cintltst/capitst.c b/icu4c/source/test/cintltst/capitst.c index 831236d9dd7..bb49f4d674c 100644 --- a/icu4c/source/test/cintltst/capitst.c +++ b/icu4c/source/test/cintltst/capitst.c @@ -73,6 +73,7 @@ void addCollAPITest(TestNode** root) /*addTest(root, &TestGetDefaultRules, "tscoll/capitst/TestGetDefaultRules");*/ addTest(root, &TestDecomposition, "tscoll/capitst/TestDecomposition"); addTest(root, &TestSafeClone, "tscoll/capitst/TestSafeClone"); + addTest(root, &TestClone, "tscoll/capitst/TestClone"); addTest(root, &TestCloneBinary, "tscoll/capitst/TestCloneBinary"); addTest(root, &TestGetSetAttr, "tscoll/capitst/TestGetSetAttr"); addTest(root, &TestBounds, "tscoll/capitst/TestBounds"); @@ -806,6 +807,124 @@ void TestSafeClone() { } } +void TestClone() { + UChar test1[6]; + UChar test2[6]; + static const UChar umlautUStr[] = {0x00DC, 0}; + static const UChar oeStr[] = {0x0055, 0x0045, 0}; + UCollator * someCollators [CLONETEST_COLLATOR_COUNT]; + UCollator * someClonedCollators [CLONETEST_COLLATOR_COUNT]; + UCollator * col = NULL; + UErrorCode err = U_ZERO_ERROR; + int8_t idx = 6; /* Leave this here to test buffer alignment in memory*/ + const char sampleRuleChars[] = "&Z < CH"; + UChar sampleRule[sizeof(sampleRuleChars)]; + + u_uastrcpy(test1, "abCda"); + u_uastrcpy(test2, "abcda"); + u_uastrcpy(sampleRule, sampleRuleChars); + + /* one default collator & two complex ones */ + someCollators[0] = ucol_open("en_US", &err); + someCollators[1] = ucol_open("ko", &err); + someCollators[2] = ucol_open("ja_JP", &err); + someCollators[3] = ucol_openRules(sampleRule, -1, UCOL_ON, UCOL_TERTIARY, NULL, &err); + if(U_FAILURE(err)) { + for (idx = 0; idx < CLONETEST_COLLATOR_COUNT; idx++) { + ucol_close(someCollators[idx]); + } + log_data_err("Couldn't open one or more collators\n"); + return; + } + + /* Check the various error & informational states: */ + + /* Null status - just returns NULL */ + if (NULL != ucol_clone(someCollators[0], NULL)) + { + log_err("FAIL: Cloned Collator failed to deal correctly with null status\n"); + } + /* error status - should return 0 & keep error the same */ + err = U_MEMORY_ALLOCATION_ERROR; + if (NULL != ucol_clone(someCollators[0], &err) || err != U_MEMORY_ALLOCATION_ERROR) + { + log_err("FAIL: Cloned Collator failed to deal correctly with incoming error status\n"); + } + err = U_ZERO_ERROR; + + /* Verify we can use this run-time calculated size */ + if (NULL == (col = ucol_clone(someCollators[0], &err)) || U_FAILURE(err)) + { + log_err("FAIL: Collator can't be cloned.\n"); + } + if (col) ucol_close(col); + + if (NULL == (col = ucol_clone(someCollators[0], &err)) || err != U_ZERO_ERROR) + { + log_err("FAIL: Cloned Collator failed to deal correctly\n"); + } + if (col) ucol_close(col); + err = U_ZERO_ERROR; + + /* Null Collator - return NULL & set U_ILLEGAL_ARGUMENT_ERROR */ + if (NULL != ucol_clone(NULL, &err) || err != U_ILLEGAL_ARGUMENT_ERROR) + { + log_err("FAIL: Cloned Collator failed to deal correctly with null Collator pointer\n"); + } + err = U_ZERO_ERROR; + + /* Test that a cloned collator doesn't accidentally use UCA. */ + col=ucol_open("de@collation=phonebook", &err); + someClonedCollators[0] = ucol_clone(col, &err); + doAssert( (ucol_greater(col, umlautUStr, u_strlen(umlautUStr), oeStr, u_strlen(oeStr))), "Original German phonebook collation sorts differently than expected"); + doAssert( (ucol_greater(someClonedCollators[0], umlautUStr, u_strlen(umlautUStr), oeStr, u_strlen(oeStr))), "Cloned German phonebook collation sorts differently than expected"); + if (!ucol_equals(someClonedCollators[0], col)) { + log_err("FAIL: Cloned German phonebook collator is not equal to original.\n"); + } + ucol_close(col); + ucol_close(someClonedCollators[0]); + + err = U_ZERO_ERROR; + + /* change orig & clone & make sure they are independent */ + + for (idx = 0; idx < CLONETEST_COLLATOR_COUNT; idx++) + { + ucol_setStrength(someCollators[idx], UCOL_IDENTICAL); + err = U_ZERO_ERROR; + ucol_close(ucol_clone(someCollators[idx], &err)); + if (err != U_ZERO_ERROR) { + log_err("FAIL: collator number %d was not allocated.\n", idx); + log_err("FAIL: status of Collator[%d] is %d (hex: %x).\n", idx, err, err); + } + + err = U_ZERO_ERROR; + someClonedCollators[idx] = ucol_clone(someCollators[idx], &err); + if (U_FAILURE(err)) { + log_err("FAIL: Unable to clone collator %d - %s\n", idx, u_errorName(err)); + continue; + } + if (!ucol_equals(someClonedCollators[idx], someCollators[idx])) { + log_err("FAIL: Cloned collator is not equal to original at index = %d.\n", idx); + } + + /* Check the usability */ + ucol_setStrength(someCollators[idx], UCOL_PRIMARY); + ucol_setAttribute(someCollators[idx], UCOL_CASE_LEVEL, UCOL_OFF, &err); + + doAssert( (ucol_equal(someCollators[idx], test1, u_strlen(test1), test2, u_strlen(test2))), "Result should be \"abcda\" == \"abCda\""); + + /* Close the original to make sure that the clone is usable. */ + ucol_close(someCollators[idx]); + + ucol_setStrength(someClonedCollators[idx], UCOL_TERTIARY); + ucol_setAttribute(someClonedCollators[idx], UCOL_CASE_LEVEL, UCOL_OFF, &err); + doAssert( (ucol_greater(someClonedCollators[idx], test1, u_strlen(test1), test2, u_strlen(test2))), "Result should be \"abCda\" >>> \"abcda\" "); + + ucol_close(someClonedCollators[idx]); + } +} + void TestCloneBinary(){ UErrorCode err = U_ZERO_ERROR; UCollator * col = ucol_open("en_US", &err); diff --git a/icu4c/source/test/cintltst/capitst.h b/icu4c/source/test/cintltst/capitst.h index 7f6f558b0d3..5c761fad895 100644 --- a/icu4c/source/test/cintltst/capitst.h +++ b/icu4c/source/test/cintltst/capitst.h @@ -74,6 +74,11 @@ **/ void TestSafeClone(void); + /** + * Test ucol_clone () + **/ + void TestClone(void); + /** * Test ucol_cloneBinary(), ucol_openBinary() **/ diff --git a/icu4c/source/test/cintltst/ccapitst.c b/icu4c/source/test/cintltst/ccapitst.c index 50750137908..aee75e6dd9d 100644 --- a/icu4c/source/test/cintltst/ccapitst.c +++ b/icu4c/source/test/cintltst/ccapitst.c @@ -64,6 +64,7 @@ static void TestJ1968(void); static void TestLMBCSMaxChar(void); #endif +static void TestConvertClone(void); #if !UCONFIG_NO_LEGACY_CONVERSION static void TestConvertSafeCloneCallback(void); #endif @@ -94,6 +95,7 @@ void addTestConvert(TestNode** root) addTest(root, &TestAlias, "tsconv/ccapitst/TestAlias"); addTest(root, &TestDuplicateAlias, "tsconv/ccapitst/TestDuplicateAlias"); addTest(root, &TestConvertSafeClone, "tsconv/ccapitst/TestConvertSafeClone"); + addTest(root, &TestConvertClone, "tsconv/ccapitst/TestConvertClone"); #if !UCONFIG_NO_LEGACY_CONVERSION addTest(root, &TestConvertSafeCloneCallback,"tsconv/ccapitst/TestConvertSafeCloneCallback"); #endif @@ -1901,6 +1903,138 @@ static void TestConvertSafeClone() } } + +static void TestConvertClone() +{ + /* one 'regular' & all the 'private stateful' converters */ + static const char *const names[] = { +#if !UCONFIG_NO_LEGACY_CONVERSION + "ibm-1047", + "ISO_2022,locale=zh,version=1", +#endif + "SCSU", +#if !UCONFIG_NO_LEGACY_CONVERSION + "HZ", + "lmbcs", + "ISCII,version=0", + "ISO_2022,locale=kr,version=1", + "ISO_2022,locale=jp,version=2", +#endif + "BOCU-1", + "UTF-7", +#if !UCONFIG_NO_LEGACY_CONVERSION + "IMAP-mailbox-name", + "ibm-1047-s390" +#else + "IMAP=mailbox-name" +#endif + }; + + char charBuffer[21]; /* Leave at an odd number for alignment testing */ + UConverter * cnv, *cnv2; + UErrorCode err; + + char *pCharBuffer; + const char *pConstCharBuffer; + const char *charBufferLimit = charBuffer + UPRV_LENGTHOF(charBuffer); + UChar uniBuffer[] = {0x0058, 0x0059, 0x005A}; /* "XYZ" */ + UChar uniCharBuffer[20]; + char charSourceBuffer[] = { 0x1b, 0x24, 0x42 }; + const char *pCharSource = charSourceBuffer; + const char *pCharSourceLimit = charSourceBuffer + sizeof(charSourceBuffer); + UChar *pUCharTarget = uniCharBuffer; + UChar *pUCharTargetLimit = uniCharBuffer + UPRV_LENGTHOF(uniCharBuffer); + const UChar * pUniBuffer; + const UChar *uniBufferLimit = uniBuffer + UPRV_LENGTHOF(uniBuffer); + int32_t idx; + + err = U_ZERO_ERROR; + cnv = ucnv_open(names[0], &err); + if(U_SUCCESS(err)) { + /* Check the various error & informational states: */ + + /* Null status - just returns NULL */ + if (NULL != ucnv_clone(cnv, NULL)) + { + log_err("FAIL: Cloned converter failed to deal correctly with null status\n"); + } + /* error status - should return 0 & keep error the same */ + err = U_MEMORY_ALLOCATION_ERROR; + if (NULL != ucnv_clone(cnv, &err) || err != U_MEMORY_ALLOCATION_ERROR) + { + log_err("FAIL: Cloned converter failed to deal correctly with incoming error status\n"); + } + err = U_ZERO_ERROR; + + /* Null buffer size pointer is ok */ + if (NULL == (cnv2 = ucnv_clone(cnv, &err)) || U_FAILURE(err)) + { + log_err("FAIL: Failed to clone.\n"); + } + ucnv_close(cnv2); + err = U_ZERO_ERROR; + + /* Null converter - return NULL & set U_ILLEGAL_ARGUMENT_ERROR */ + if (NULL != ucnv_clone(NULL, &err) || err != U_ILLEGAL_ARGUMENT_ERROR) + { + log_err("FAIL: Cloned converter failed to deal correctly with null converter pointer\n"); + } + + ucnv_close(cnv); + } + + /* Do these cloned converters work at all - shuffle UChars to chars & back again..*/ + for (idx = 0; idx < UPRV_LENGTHOF(names); idx++) + { + err = U_ZERO_ERROR; + cnv = ucnv_open(names[idx], &err); + if(U_FAILURE(err)) { + log_data_err("ucnv_open(\"%s\") failed - %s\n", names[idx], u_errorName(err)); + continue; + } + + cnv2 = ucnv_clone(cnv, &err); + + /* close the original immediately to make sure that the clone works by itself */ + ucnv_close(cnv); + + pCharBuffer = charBuffer; + pUniBuffer = uniBuffer; + + ucnv_fromUnicode(cnv2, + &pCharBuffer, + charBufferLimit, + &pUniBuffer, + uniBufferLimit, + NULL, + TRUE, + &err); + if(U_FAILURE(err)){ + log_err("FAIL: cloned converter failed to do fromU conversion. Error: %s\n",u_errorName(err)); + } + ucnv_toUnicode(cnv2, + &pUCharTarget, + pUCharTargetLimit, + &pCharSource, + pCharSourceLimit, + NULL, + TRUE, + &err + ); + + if(U_FAILURE(err)){ + log_err("FAIL: cloned converter failed to do toU conversion. Error: %s\n",u_errorName(err)); + } + + pConstCharBuffer = charBuffer; + if (uniBuffer [0] != ucnv_getNextUChar(cnv2, &pConstCharBuffer, pCharBuffer, &err)) + { + log_err("FAIL: Cloned converter failed to do conversion. Error: %s\n",u_errorName(err)); + } + ucnv_close(cnv2); + } +} + static void TestCCSID() { #if !UCONFIG_NO_LEGACY_CONVERSION UConverter *cnv; -- 2.40.0