Issue 295090043: CompactDecimalDataCache - C++ (Closed)
|
Created:
7 years, 11 months ago by sffc Modified:
7 years, 10 months ago Base URL:
http://source.icu-project.org/repos/icu/icu/trunk/source/ Visibility:
Public. |
DescriptionUploading initial version of C++ CompactDecimalDataCache (using ResourceSink).
Patch Set 1 #Patch Set 2 : Updating with changes from Java version. #
Total comments: 2
Patch Set 3 : Small logic changes for Java/C++ consistency. #
Total comments: 20
Patch Set 4 : Updating with Markus's first round of feedback. #
Total comments: 2
Patch Set 5 : Updating with Markus's second round of feedback. #
Total comments: 2
MessagesTotal messages: 13
Updating with changes from Java version.
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/20001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/20001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:638: } IMPORTANT: This logic is different from the Java version. Java's logic caused some tests to fail in C++. I've thought it through again, and these modified if statements make the C++ tests pass, but please double-check to make sure that my logic is right. The equivalent spot in the old code is in the function "initCDFLocaleData". I may need to push an update to the Java version once we get this hammered down. Note: The variable "isFallback" is set to true iff nsName!=latn and we are evaluating latn. At one point I had the variable named "isLatinFallback", and then I wrongfully changed it to "isLatin" in Java, when I should have changed it to "isFallback". https://codereview.appspot.com/295090043/diff/20001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:788: if (unit->isSet() && overwrite == FALSE) { I'll change this to "!overwrite"
Sign in to reply to this message.
Here's the code review for CompactDecimalDataCache. I've been waiting on this one since Wednesday, but it looks like I never added the reviewers. >_<
Sign in to reply to this message.
Small logic changes for Java/C++ consistency.
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:3: * Copyright (C) 1997-2015, International Business Machines Corporation and * adjust to 1997-2016, and please remove the trailing (spaces star) on this line and next https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:101: uprv_memset(divisors, 0.0, sizeof(divisors)); memset just takes a byte value. Please change 0.0 to just 0. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:111: return (unitsByVariant == NULL || unitsByVariant->count == 0) ? TRUE : FALSE; return unitsByVariant == NULL || unitsByVariant->count == 0; https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:587: // else if (uprv_strcmp(key, gCurrencyFormat) == 0) { Please pull the preceding } to this line: // } else if (... https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:590: else { Please pull the preceding } (from the comment) to this line. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:726: if (U_FAILURE(status)) return; Should you not check for U_MISSING_RESOURCE_ERROR and in that case continue with Latin (and not isFallback)? https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:733: path.append(gNumberElementsTag, status) I would use one string literal here rather than work with CharString. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:740: // TODO: Markus: Can you confirm that this is the right way to do this in C++? Not sure what "this" is. It is ok to use the bogus state of a UnicodeString where you use null in Java. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:790: if (unit->isSet() && overwrite == FALSE) { if (unit->isSet() && !overwrite) { https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:883: for (;element != NULL; element = uhash_nextElement(result->unitsByVariant, &pos)) { Simpler: const UHashElement* element; while ((element = uhash_nextElement(result->unitsByVariant, &pos)) != NULL) {
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:3: * Copyright (C) 1997-2015, International Business Machines Corporation and * On 2016/05/13 17:58:50, markus.icu wrote: > adjust to 1997-2016, and please remove the trailing (spaces star) on this line > and next Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:101: uprv_memset(divisors, 0.0, sizeof(divisors)); On 2016/05/13 17:58:50, markus.icu wrote: > memset just takes a byte value. Please change 0.0 to just 0. Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:111: return (unitsByVariant == NULL || unitsByVariant->count == 0) ? TRUE : FALSE; On 2016/05/13 17:58:51, markus.icu wrote: > return unitsByVariant == NULL || unitsByVariant->count == 0; Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:587: // else if (uprv_strcmp(key, gCurrencyFormat) == 0) { On 2016/05/13 17:58:51, markus.icu wrote: > Please pull the preceding } to this line: > // } else if (... Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:590: else { On 2016/05/13 17:58:50, markus.icu wrote: > Please pull the preceding } (from the comment) to this line. Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:726: if (U_FAILURE(status)) return; On 2016/05/13 17:58:50, markus.icu wrote: > Should you not check for U_MISSING_RESOURCE_ERROR and in that case continue with > Latin (and not isFallback)? Good idea. I'll change this in both C++ and Java. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:733: path.append(gNumberElementsTag, status) On 2016/05/13 17:58:50, markus.icu wrote: > I would use one string literal here rather than work with CharString. Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:740: // TODO: Markus: Can you confirm that this is the right way to do this in C++? On 2016/05/13 17:58:50, markus.icu wrote: > Not sure what "this" is. > It is ok to use the bogus state of a UnicodeString where you use null in Java. Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:790: if (unit->isSet() && overwrite == FALSE) { On 2016/05/13 17:58:50, markus.icu wrote: > if (unit->isSet() && !overwrite) { Done. https://codereview.appspot.com/295090043/diff/40001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:883: for (;element != NULL; element = uhash_nextElement(result->unitsByVariant, &pos)) { On 2016/05/13 17:58:50, markus.icu wrote: > Simpler: > > const UHashElement* element; > while ((element = uhash_nextElement(result->unitsByVariant, &pos)) != NULL) { Done.
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/60001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/60001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:724: // Silently ignore and use Latin It seems like isFallback should be FALSE here. Or did the original logic behave like isFallback=TRUE in this case? Please use uppercase TRUE/FALSE with UBool.
Sign in to reply to this message.
Updating with Markus's second round of feedback.
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/80001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/80001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:3: * Copyright (C) 2016 and later: Unicode, Inc. and others. Please revert these two lines to what you had in ps4. See my reply to "[icu-core] Copyright Update".
Sign in to reply to this message.
https://codereview.appspot.com/295090043/diff/60001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/60001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:724: // Silently ignore and use Latin On 2016/05/30 18:26:55, markus.icu wrote: > It seems like isFallback should be FALSE here. Or did the original logic behave > like isFallback=TRUE in this case? > > Please use uppercase TRUE/FALSE with UBool. The isFallback flag is relevant only if data was consumed, so the distinction here is benign. I switched to uppercase TRUE/FALSE, sorry about that. https://codereview.appspot.com/295090043/diff/80001/i18n/compactdecimalformat... File i18n/compactdecimalformat.cpp (right): https://codereview.appspot.com/295090043/diff/80001/i18n/compactdecimalformat... i18n/compactdecimalformat.cpp:3: * Copyright (C) 2016 and later: Unicode, Inc. and others. On 2016/05/31 20:32:58, markus.icu wrote: > Please revert these two lines to what you had in ps4. See my reply to > "[icu-core] Copyright Update". When I ran "svn update" I got a merge conflict on these lines. This is the version of the copyright header currently in trunk.
Sign in to reply to this message.
LGTM Please wait with committing until after Yoshito has reverted the copyright mis-update, then take the IBM copyright lines back to what you had in ps4, then commit.
Sign in to reply to this message.
On 2016/05/31 21:22:18, markus.icu wrote: > LGTM > > Please wait with committing until after Yoshito has reverted the copyright > mis-update, then take the IBM copyright lines back to what you had in ps4, then > commit. Submitted as r38779.
Sign in to reply to this message.
|
