ticket:13513 ICU Number Parsing (Java)

ticket:13513 ICU Number Parsing (Java)

[sffc, 2018-03-01 23:02:34]

https://codereview.appspot.com/340310043/diff/1/icu4j/main/tests/core/src/com...
File
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:1228:
// P fails here because this currency name is in the Trie only, but it has the
same prefix as the non-Trie currency
I feel like this has the potential to be problematic.  I haven't thought deeply
about potential consequences.  What do you think?

[andy.heninger, 2018-03-19 02:39:33]

aa

https://codereview.appspot.com/340310043/diff/1/icu4c/source/test/testdata/nu...
File icu4c/source/test/testdata/numberformattestspecification.txt (right):

https://codereview.appspot.com/340310043/diff/1/icu4c/source/test/testdata/nu...
icu4c/source/test/testdata/numberformattestspecification.txt:955: // C and P
consume the '9' as a digit and assumes number is negative
P is not described in the doc describing the file format.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixPatternProvider.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixPatternProvider.java:5:
public interface AffixPatternProvider {
Add some description, since it appears to be important.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/AffixMatcher.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/AffixMatcher.java:47:
private static boolean isInteresting(
Add a comment saying determines if a matcher needs to be built.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/AffixMatcher.java:91:
ArrayList<AffixMatcher> matchers = new ArrayList<AffixMatcher>(6);
should be 9, not 6

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/InfinityMatcher.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/InfinityMatcher.java:45:
return "<PercentMatcher>";
wrong name

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParseMatcher.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParseMatcher.java:8:
* Given a string, there should NOT be more than one way to consume the string
with the same matcher
maybe the top comment should be What Is a NumberPaarserMatcther, then discuss
exponential time considerations.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParseMatcher.java:45:
* Should return a set representing all possible chars (UTF-16 code units) that
could be the first
Comment out of date

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParserImpl.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParserImpl.java:70:
public static NumberParserImpl createParserFromPattern(
add comment for test only functions, saying so.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParserImpl.java:228:
if (!isStrict
nested !isStrict why?

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParserImpl.java:288:
* @param ignoreCase
doesn't match actual param

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/NumberParserImpl.java:331:
public void setComparator(Comparator<ParsedNumber> comparator) {
Shane asks if this is more confusing than it's worth.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/StringSegment.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/StringSegment.java:76:
public int getCodePoint() {
supplemental handling is wrong, but was fixed already.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/SymbolMatcher.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/SymbolMatcher.java:15:
// TODO: Implement this class using only UnicodeSet and not String?
UnicodeSets can hold strings as well as code points. Not sure that it would make
things any simpler, though.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/UnicodeSetStaticCache.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/UnicodeSetStaticCache.java:31:
// - PERIOD is a superset of SCRICT_PERIOD
Typo SCRICT

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/UnicodeSetStaticCache.java:98:
// TODO: Re-generate these sets from the UCD. They probably haven't been updated
in a while.
Could you have a test that fails if it looks like any of these need updating?

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormat.java (right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormat.java:277: * read and
write at the same time.
We should describe more fully what is safe and not safe for use of the old API.
It may not be quite as bad as this comment implies.

What the old API was trying to accomplish with synchronized setters is confusing
to me. It's not consistent with ICU4C threading, which is what I thought the old
Java API was trying to do.

https://codereview.appspot.com/340310043/diff/1/icu4j/main/tests/core/src/com...
File
icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/DecimalQuantityTest.java
(right):

https://codereview.appspot.com/340310043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/DecimalQuantityTest.java:525:
quantity.setToBigInteger(new BigInteger("9223372036854775808"));
How about -9223372036854775808?