Issue 293250043: CompactDecimalDataCache - Java (Closed)
|
Created:
8 years ago by sffc Modified:
7 years, 11 months ago Base URL:
http://source.icu-project.org/repos/icu/icu4j/trunk/ Visibility:
Public. |
DescriptionFirst draft of the CompactDecimalDataCache data loading transformation.
Patch Set 1 #Patch Set 2 : Small logic fix #
Total comments: 35
Patch Set 3 : Updating with Markus's first round of feedback #
Total comments: 14
Patch Set 4 : Updating with Markus's second round of feedback #
Total comments: 16
Patch Set 5 : Updating with Markus's third round of feedback #
Total comments: 1
Patch Set 6 : Revising logic for Serbian-Long test case #Patch Set 7 : One more try at the logic. #
Total comments: 2
Patch Set 8 : Adding handler for resource not found. #
Total comments: 2
Patch Set 9 : One last update before submitting. #MessagesTotal messages: 29
Here is the Java CompactDecimalDataCache code.
Sign in to reply to this message.
Small logic fix
Sign in to reply to this message.
Small logic fix
Sign in to reply to this message.
Small logic fix
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:200: // and shortData is already populated, perhaps from deeper in Latin Double-check that this logic produces the same result as the old logic.
Sign in to reply to this message.
Initial feedback. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:11: import java.util.Map.Entry; Please use Map.Entry in the code and remove this import. I think it's more understandable that way. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:70: static class Data { Probably private? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:82: return this.units == null || this.units.isEmpty(); "this." is only needed in the constructor when the parameters have the same name. Please otherwise remove. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:97: static class DataBundle { Probably private? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:126: private ULocale locale; // The locale we are traversing (for logging purposes) Please add a TODO to remember to remove this. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:138: public CmptDecDataSink(DataBundle dataBundle, ULocale locale) { CompactDecimalDataSink would be nicer. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: // SPECIAL CASE: Never consume the root if we're on our first traversal Adjust, for example: // Skip root data while loading non-latn so that we fall back to latn then root. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:148: UResource.Table lvl1table = value.getTable(); Names like lvl1table are hard. You are also off by one level from your diagram above: lvl1table is the "top" table with the nsName as its key. I suggest you use more descriptive names like nsTable, patterns, formats, divisors, pluralPatterns -- and drop the "<-- lvl 1" suffixes from your diagram. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:152: String lvl1 = null; I would use enums for these; that would port more nicely to C++ too. There should already be enums for some of these distinctions (e.g., width long vs. abbreviated) defined on the CompactDecimalFormat API. You can make private enums where missing. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:196: // 1) Don't consume any data if we're in Latin fallback and it was already remove "fallback" https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:203: continue; Please use { block braces } for if/else/loop bodies. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:215: destination.setFromLocaleNumberingSystem(!isLatinFallback); what if nsName==latn? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:265: public void setIsLatinFallback(boolean isLatinFallback) { Please rename the field to isLatin, initialize it in the constructor as isLatin = false; then change this method to setIsLatin() { isLatin = true; } https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:294: // Now load the Latin fallback if required Looks like this does not work -- it won't see the root symbols if nsName==latn. I think you need to invert which part is conditional: if (!nsName.equals(LATIN_NUMBERING_SYSTEM)) { load non-latn data } sink.setIsLatin(); load latn data
Sign in to reply to this message.
Updating with Markus's first round of feedback
Sign in to reply to this message.
Updated with Markus's first round of feedback. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:11: import java.util.Map.Entry; On 2016/04/29 23:47:38, markus.icu wrote: > Please use Map.Entry in the code and remove this import. I think it's more > understandable that way. Done. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:70: static class Data { On 2016/04/29 23:47:38, markus.icu wrote: > Probably private? CompactDecimalFormat has references to CompactDecimalDataCache.Data (as a data type), so it can't be private. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:82: return this.units == null || this.units.isEmpty(); On 2016/04/29 23:47:38, markus.icu wrote: > "this." is only needed in the constructor when the parameters have the same > name. Please otherwise remove. Yes... the rest of ICU omits the "this" keyword, so I should be consistent. Thanks for catching it. Fixed. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:97: static class DataBundle { On 2016/04/29 23:47:38, markus.icu wrote: > Probably private? Like Data, CompactDecimalFormat has references to CompactDecimalDataCache.DataBundle (as a data type), so it can't be private. I'll make the constructors private though. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:126: private ULocale locale; // The locale we are traversing (for logging purposes) On 2016/04/29 23:47:38, markus.icu wrote: > Please add a TODO to remember to remove this. It's actually for messages in exception text. I'll clarify it. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:138: public CmptDecDataSink(DataBundle dataBundle, ULocale locale) { On 2016/04/29 23:47:38, markus.icu wrote: > CompactDecimalDataSink would be nicer. Done. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: // SPECIAL CASE: Never consume the root if we're on our first traversal On 2016/04/29 23:47:38, markus.icu wrote: > Adjust, for example: > // Skip root data while loading non-latn so that we fall back to latn then root. This comment here, and the one on line 195, are indicating the two places where the code logic is enforcing assumptions about the fallback mechanisms in the data files. I'd like to make these two places obvious to people in the future reading this code. Any suggestions on how to do that? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:148: UResource.Table lvl1table = value.getTable(); On 2016/04/29 23:47:38, markus.icu wrote: > Names like lvl1table are hard. You are also off by one level from your diagram > above: lvl1table is the "top" table with the nsName as its key. > > I suggest you use more descriptive names like nsTable, patterns, formats, > divisors, pluralPatterns -- and drop the "<-- lvl 1" suffixes from your diagram. I renamed the variables and updated the figure. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:152: String lvl1 = null; On 2016/04/29 23:47:38, markus.icu wrote: > I would use enums for these; that would port more nicely to C++ too. > > There should already be enums for some of these distinctions (e.g., width long > vs. abbreviated) defined on the CompactDecimalFormat API. You can make private > enums where missing. Ok, I made them enums. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:196: // 1) Don't consume any data if we're in Latin fallback and it was already On 2016/04/29 23:47:38, markus.icu wrote: > remove "fallback" Done. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:203: continue; On 2016/04/29 23:47:38, markus.icu wrote: > Please use { block braces } for if/else/loop bodies. Done. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:215: destination.setFromLocaleNumberingSystem(!isLatinFallback); On 2016/04/29 23:47:38, markus.icu wrote: > what if nsName==latn? I changed the name of the field to "fromLatin" (and flipped the boolean). https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:265: public void setIsLatinFallback(boolean isLatinFallback) { On 2016/04/29 23:47:38, markus.icu wrote: > Please rename the field to isLatin, initialize it in the constructor as > isLatin = false; > then change this method to > setIsLatin() { > isLatin = true; > } I renamed the field. Since the sink is a private internal class, I think now it feels a bit verbose to have the setter. I changed it to a regular field. Is that ok? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:294: // Now load the Latin fallback if required On 2016/04/29 23:47:38, markus.icu wrote: > Looks like this does not work -- it won't see the root symbols if nsName==latn. > I think you need to invert which part is conditional: > > if (!nsName.equals(LATIN_NUMBERING_SYSTEM)) { > load non-latn data > } > sink.setIsLatin(); > load latn data You're absolutely right. This is the reason why I had "fallback" in all the variable names above, but looking back at the old logic, it will skip the first data load, not the second data load, if nsName==latin. I'll adjust this.
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:82: return this.units == null || this.units.isEmpty(); On 2016/05/01 21:29:20, sffc wrote: > On 2016/04/29 23:47:38, markus.icu wrote: > > "this." is only needed in the constructor when the parameters have the same > > name. Please otherwise remove. > > Yes... the rest of ICU omits the "this" keyword, so I should be consistent. > Thanks for catching it. Fixed. Not fixed. Did you forget to uploaded your latest version? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: // SPECIAL CASE: Never consume the root if we're on our first traversal I suggest you use something like my original suggestion. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:265: public void setIsLatinFallback(boolean isLatinFallback) { Field instead of setter is fine. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:71: public static class Data { So why public now? Can you keep it package-private? https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:94: public static class DataBundle { Ditto why public now? Can you keep it package-private? https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: UResource.Table patternsTable = value.getTable(); You are still off by one level with these names, which is very confusing. This value.getTable() returns the "beng" or "latn" etc. table. I had suggested "nsTable" for the Table variable. In other words, the name of the variable should be similar to the name of the resource it represents. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:153: if (key.contentEquals(PATTERNS_LONG)) { You are retesting the key even if you had a match. Please use PatternsTableKey patternsTableKey; if (short) { patternsTableKey = PatternsTableKey.PATTERNS_SHORT; } else if (long) { patternsTableKey = PatternsTableKey.PATTERNS_LONG; } else { continue; } https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:167: if (key.contentEquals(CURRENCY_FORMAT)) { ditto https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:180: else if (patternsTableKey == PatternsTableKey.PATTERNS_SHORT Much nicer if you put the } and the "else if" on the same line. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:288: // Set the "isFallback" flag so that the sink knows the Latin traversal is a fallback. Remove this line.
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/293250043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:82: return this.units == null || this.units.isEmpty(); On 2016/05/02 23:19:18, markus.icu wrote: > On 2016/05/01 21:29:20, sffc wrote: > > On 2016/04/29 23:47:38, markus.icu wrote: > > > "this." is only needed in the constructor when the parameters have the same > > > name. Please otherwise remove. > > > > Yes... the rest of ICU omits the "this" keyword, so I should be consistent. > > Thanks for catching it. Fixed. > > Not fixed. Did you forget to uploaded your latest version? Yes, sorry, it's in the newest version. https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: // SPECIAL CASE: Never consume the root if we're on our first traversal On 2016/05/02 23:19:18, markus.icu wrote: > I suggest you use something like my original suggestion. Done. I left in "SPECIAL CASE"; is that alright, or should I remove it as well? https://codereview.appspot.com/293250043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:265: public void setIsLatinFallback(boolean isLatinFallback) { On 2016/05/02 23:19:19, markus.icu wrote: > Field instead of setter is fine. Acknowledged. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:71: public static class Data { On 2016/05/02 23:19:19, markus.icu wrote: > So why public now? Can you keep it package-private? Done. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:94: public static class DataBundle { On 2016/05/02 23:19:19, markus.icu wrote: > Ditto why public now? Can you keep it package-private? Done. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:145: UResource.Table patternsTable = value.getTable(); On 2016/05/02 23:19:19, markus.icu wrote: > You are still off by one level with these names, which is very confusing. This > value.getTable() returns the "beng" or "latn" etc. table. I had suggested > "nsTable" for the Table variable. In other words, the name of the variable > should be similar to the name of the resource it represents. The way I am thinking about it, "patternsTable" is the table you traverse to get a list of patterns, "formatsTable" is the table you traverse to get a list of formats, and so on. I would find it confusing if I was traversing "patternsTable" and I got formats as my entries. :) Names that would be more clear, though long, would be something like, "patternsForNumberingSystemTable", "formatsForPatternTable", "powersOfTenForFormatTable", and "pluralVariantsForPowerOfTenTable". Does that sound good? https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:153: if (key.contentEquals(PATTERNS_LONG)) { On 2016/05/02 23:19:19, markus.icu wrote: > You are retesting the key even if you had a match. Please use > > PatternsTableKey patternsTableKey; > if (short) { > patternsTableKey = PatternsTableKey.PATTERNS_SHORT; > } else if (long) { > patternsTableKey = PatternsTableKey.PATTERNS_LONG; > } else { > continue; > } Done. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:167: if (key.contentEquals(CURRENCY_FORMAT)) { On 2016/05/02 23:19:19, markus.icu wrote: > ditto Acknowledged. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:180: else if (patternsTableKey == PatternsTableKey.PATTERNS_SHORT On 2016/05/02 23:19:19, markus.icu wrote: > Much nicer if you put the } and the "else if" on the same line. Done, but I find it more crowded with them on the same line. https://codereview.appspot.com/293250043/diff/80001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:288: // Set the "isFallback" flag so that the sink knows the Latin traversal is a fallback. On 2016/05/02 23:19:19, markus.icu wrote: > Remove this line. Done.
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:128: * latn{ <-- top (numbering system table) I think it would help if you changed the comments here to the corresponding variable names. latn{ <-- patternsTable (for numbering system) patternsLong{ <-- formatsTable decimalFormat{ <-- powersOfTenTable 1000{ <-- pluralVariantsTable one{"0 thousand"} <-- pattern https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:232: String pluralVariant = key.toString(); Please add a TODO somewhere like // TODO: Use StandardPlural rather than String. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:233: String template = value.toString(); Optional: s/template/pattern/ https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:242: if (numZeros == -1) continue; Use { block braces }. Optional: Simpler condition, if (numZeros < 0) https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:286: sink.isLatin = false; Unnecessary assignment to the initial value. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:340: boolean result = saveUnit(new DecimalFormat.Unit(prefix, suffix), pluralVariant, idx, destination.units, overwrite); Optional: s/result/saved/ https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:481: if (!overwrite && byBase[idx] != null) return false; { block braces }
Sign in to reply to this message.
Updating with Markus's third round of feedback
Sign in to reply to this message.
Updating with Markus's third round of feedback
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:128: * latn{ <-- top (numbering system table) On 2016/05/03 18:31:19, markus.icu wrote: > I think it would help if you changed the comments here to the corresponding > variable names. > latn{ <-- patternsTable (for numbering system) > patternsLong{ <-- formatsTable > decimalFormat{ <-- powersOfTenTable > 1000{ <-- pluralVariantsTable > one{"0 thousand"} <-- pattern Sounds good. Done. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:232: String pluralVariant = key.toString(); On 2016/05/03 18:31:20, markus.icu wrote: > Please add a TODO somewhere like > // TODO: Use StandardPlural rather than String. Done. What is StandardPlural? https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:233: String template = value.toString(); On 2016/05/03 18:31:19, markus.icu wrote: > Optional: s/template/pattern/ "pattern" means something else in this context. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:242: if (numZeros == -1) continue; On 2016/05/03 18:31:19, markus.icu wrote: > Use { block braces }. > > Optional: Simpler condition, if (numZeros < 0) Done. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:286: sink.isLatin = false; On 2016/05/03 18:31:20, markus.icu wrote: > Unnecessary assignment to the initial value. Acknowledged. I just think it makes the code more clear. I imagine that the compiler will optimize it out. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:340: boolean result = saveUnit(new DecimalFormat.Unit(prefix, suffix), pluralVariant, idx, destination.units, overwrite); On 2016/05/03 18:31:19, markus.icu wrote: > Optional: s/result/saved/ Done. Also added curly braces to line 341. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:481: if (!overwrite && byBase[idx] != null) return false; On 2016/05/03 18:31:19, markus.icu wrote: > { block braces } Done. https://codereview.appspot.com/293250043/diff/140001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/140001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:284: ICUResourceBundle r = (ICUResourceBundle) UResourceBundle.getBundleInstance(ICUData.ICU_BASE_NAME, I changed this to ICUData.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:232: String pluralVariant = key.toString(); On 2016/05/04 00:42:27, sffc wrote: > What is StandardPlural? It's in com.ibm.icu.impl -- an enum with parsing. https://codereview.appspot.com/293250043/diff/100001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:233: String template = value.toString(); On 2016/05/04 00:42:27, sffc wrote: > "pattern" means something else in this context. No, patternsLong is a tree of patterns keyed by paths of options. That's why it is called that in the data. But I don't care strongly about the variable name.
Sign in to reply to this message.
Revising logic for Serbian-Long test case
Sign in to reply to this message.
Fixing logic in Java to be consistent with C++ changes and make Serbian test cases pass.
Sign in to reply to this message.
One more try at the logic.
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/180001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/180001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:291: if (!nsName.equals(LATIN_NUMBERING_SYSTEM)) { It might be possible to simplify the sink and eliminate the isLatin and isFallback flags if you bring part of the logic of the original method into here instead of the sink. In particular, you could use the sink just to enumerate the NumberingSystem and store the shortData, longData and shortCurrencyData on fields inside the sink object. Then on this method you could do something like: if (not latin) { r.getAllItemsWithFallback(not_latin_path, sink); shortData = sink.shortData; longData = sink.longData; shortCurrencyData = sink.shortCurrencyData; } r.getAllItemsWithFallback(latin_path, sink); if (shortData == null) { shortData = sink.shortData; if (longData == null) { longData = sink.longData; } } if (shortCurrencyData == null) { shortCurrencyData = sink.shortCurrencyData; } Check it out if you like the idea :)
Sign in to reply to this message.
LGTM https://codereview.appspot.com/293250043/diff/180001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/180001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:291: if (!nsName.equals(LATIN_NUMBERING_SYSTEM)) { FYI: I have not thought through which version is shorter, simpler, and/or better avoids reading data that it won't use.
Sign in to reply to this message.
Adding handler for resource not found.
Sign in to reply to this message.
Adding MissingResourceException try/catch for C++ consistency.
Sign in to reply to this message.
LGTM in case this is really still pending
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/200001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/200001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:300: // Set the "isFallback" flag for when we read Latin Please see my question about this in the C++ review.
Sign in to reply to this message.
One last update before submitting.
Sign in to reply to this message.
On 2016/05/31 17:33:38, sffc wrote: > One last update before submitting. Submitted as r38773.
Sign in to reply to this message.
https://codereview.appspot.com/293250043/diff/200001/main/classes/core/src/co... File main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java (right): https://codereview.appspot.com/293250043/diff/200001/main/classes/core/src/co... main/classes/core/src/com/ibm/icu/text/CompactDecimalDataCache.java:300: // Set the "isFallback" flag for when we read Latin On 2016/05/30 18:27:51, markus.icu wrote: > Please see my question about this in the C++ review. Acknowledged.
Sign in to reply to this message.
|
