Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(184)

Issue 329960043: ticket:13177 NumberFormat2 from Shane's branch

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 4 days ago by andy.heninger
Modified:
3 days, 9 hours ago
Reviewers:
sffc
Base URL:
svn+ssh://source.icu-project.org/repos/icu/trunk/icu4j/
Visibility:
Public.

Description

ticket:13177 NumberFormat2 from Shane's branch

Patch Set 1 #

Total comments: 26
Unified diffs Side-by-side diffs Delta from patch set Stats (+7683 lines, -1303 lines) Patch
M main/classes/core/src/com/ibm/icu/impl/number/AffixPatternUtils.java View 12 chunks +126 lines, -31 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity.java View 3 chunks +13 lines, -7 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity1.java View 3 chunks +24 lines, -15 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/FormatQuantityBCD.java View 3 chunks +15 lines, -3 lines 0 comments Download
A main/classes/core/src/com/ibm/icu/impl/number/LdmlPatternInfo.java View 1 chunk +449 lines, -0 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/Modifier.java View 4 chunks +8 lines, -9 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/ModifierHolder.java View 2 chunks +6 lines, -14 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/NumberStringBuilder.java View 5 chunks +39 lines, -0 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/Parse.java View 2 chunks +24 lines, -1 line 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/PatternString.java View 4 chunks +157 lines, -455 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/Properties.java View 8 chunks +0 lines, -17 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/Rounder.java View 1 chunk +2 lines, -1 line 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/RoundingUtils.java View 1 chunk +11 lines, -0 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/formatters/CompactDecimalFormat.java View 4 chunks +1 line, -11 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/formatters/PaddingFormat.java View 3 chunks +8 lines, -8 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/formatters/PositiveDecimalFormat.java View 1 chunk +4 lines, -16 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/formatters/ScientificFormat.java View 1 chunk +2 lines, -1 line 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/modifiers/ConstantAffixModifier.java View 3 chunks +2 lines, -10 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/modifiers/ConstantMultiFieldModifier.java View 3 chunks +5 lines, -11 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/modifiers/SimpleModifier.java View 4 chunks +43 lines, -37 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/impl/number/rounders/SignificantDigitsRounder.java View 8 chunks +4 lines, -78 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/text/CompactDecimalFormat.java View 3 chunks +23 lines, -8 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/text/DecimalFormat.java View 31 chunks +64 lines, -166 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java View 6 chunks +73 lines, -7 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/text/NumberingSystem.java View 1 chunk +2 lines, -0 lines 0 comments Download
A main/classes/core/src/com/ibm/icu/util/Dimensionless.java View 1 chunk +21 lines, -0 lines 0 comments Download
M main/classes/core/src/com/ibm/icu/util/MeasureUnit.java View 2 chunks +9 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/NumberFormatter.java View 1 chunk +686 lines, -0 lines 6 comments Download
A main/classes/core/src/newapi/demo.java View 1 chunk +65 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/AffixPatternProvider.java View 1 chunk +28 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/CompactData.java View 1 chunk +278 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/CompactImpl.java View 1 chunk +116 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/CurrencySpacingEnabledModifier.java View 1 chunk +175 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/CustomSymbolCurrency.java View 1 chunk +63 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/DataUtils.java View 1 chunk +69 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/GroupingImpl.java View 1 chunk +136 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/IntegerWidthImpl.java View 1 chunk +29 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/MacroProps.java View 1 chunk +107 lines, -0 lines 1 comment Download
A main/classes/core/src/newapi/impl/MeasureData.java View 1 chunk +66 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/MicroProps.java View 1 chunk +60 lines, -0 lines 4 comments Download
A main/classes/core/src/newapi/impl/MultiplierImpl.java View 1 chunk +41 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/MurkyModifier.java View 1 chunk +416 lines, -0 lines 2 comments Download
A main/classes/core/src/newapi/impl/NotationImpl.java View 1 chunk +83 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/NumberFormatterImpl.java View 1 chunk +330 lines, -0 lines 7 comments Download
A main/classes/core/src/newapi/impl/NumberPropertyMapper.java View 1 chunk +449 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/PaddingImpl.java View 1 chunk +111 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/PositiveDecimalImpl.java View 1 chunk +116 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/QuantityChain.java View 1 chunk +12 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/QuantityDependentModOuter.java View 1 chunk +39 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/RoundingImpl.java View 1 chunk +447 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/ScientificImpl.java View 1 chunk +146 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/SkeletonBuilder.java View 1 chunk +551 lines, -0 lines 0 comments Download
A main/classes/core/src/newapi/impl/Worker1.java View 1 chunk +265 lines, -0 lines 6 comments Download
M main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt View 12 chunks +38 lines, -31 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/CompactDecimalFormatTest.java View 5 chunks +7 lines, -6 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatAPI.java View 1 chunk +1 line, -2 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatAPIC.java View 1 chunk +1 line, -1 line 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java View 3 chunks +15 lines, -0 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbolsC.java View 1 chunk +1 line, -1 line 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/MeasureUnitTest.java View 3 chunks +11 lines, -8 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatDataDrivenTest.java View 6 chunks +159 lines, -108 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatRegressionTest.java View 2 chunks +3 lines, -2 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java View 21 chunks +53 lines, -84 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTestCases.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/format/NumberRegressionTests.java View 6 chunks +21 lines, -10 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/number/AffixPatternUtilsTest.java View 4 chunks +39 lines, -1 line 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/number/FormatQuantityTest.java View 1 chunk +2 lines, -1 line 0 comments Download
A main/tests/core/src/com/ibm/icu/dev/test/number/MurkyModifierTest.java View 1 chunk +60 lines, -0 lines 0 comments Download
A main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterTest.java View 1 chunk +1250 lines, -0 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/number/PropertiesTest.java View 2 chunks +0 lines, -6 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/number/RounderTest.java View 1 chunk +0 lines, -134 lines 0 comments Download
M main/tests/core/src/com/ibm/icu/dev/test/serializable/SerializableTestUtility.java View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 2
andy.heninger
https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/NumberFormatter.java File main/classes/core/src/newapi/NumberFormatter.java (right): https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/NumberFormatter.java#newcode3 main/classes/core/src/newapi/NumberFormatter.java:3: package newapi; What is the ultimate package? com.ibm.icu.text, along ...
4 days, 16 hours ago (2017-08-15 21:00:04 UTC) #1
sffc
3 days, 9 hours ago (2017-08-17 04:24:36 UTC) #2
Thanks for the review! I addressed some of the comments. I'll get to the rest
soon.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/NumberFormatter.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:3: package newapi;
On 2017/08/15 21:00:03, andy.heninger wrote:
> What is the ultimate package? com.ibm.icu.text, along with all the other
> formatting APIs?

I was planning on com.ibm.icu.text, along with all the other formatting APIs,
unless we want to declare a whole new package like com.ibm.icu.number (which
could also simplify the code by eliminating the need for the duplicated classes
in com.ibm.icu.impl).

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MicroProps.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:43: if (frozen) {
On 2017/08/15 21:00:03, andy.heninger wrote:
> Check who freezes, can there be races?

There shouldn't be any races.  I moved the "freeze" to the constructor to make
it more clear.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:55: throw new
AssertionError();
On 2017/08/15 21:00:03, andy.heninger wrote:
> Could throw new AssertionError(e);
> to give more info should it happen.

Done.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MurkyModifier.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MurkyModifier.java:30: final boolean isStrong;
On 2017/08/15 21:00:03, andy.heninger wrote:
> What does 'strong' mean?

See the documentation in the Modifier.java interface.  If a modifier is *not*
strong, it can be bubbled up and handled by the future range format.  For
example, scientific notation is strong, but not compact notation.

3E3-5E3
3-5K

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/NumberFormatterImpl.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:80: volatile
AtomicInteger callCount;
On 2017/08/15 21:00:03, andy.heninger wrote:
> change to
>   volatile long callCount;
>   static final AtomicLongFieldUpdater<NumberFormatterImpl> callCountUpdater =
>           AtomicLongFieldUpdater.newUpdater(NumberFormatterImpl.class,
> "callCount");
> 
> Also change usage, noted below.

Done.  Thanks!

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:224: }
On 2017/08/15 21:00:03, andy.heninger wrote:
> This lazy init is not safe, racing threads can create duplicate
AtomicIntegers.
> Using an AtomicLongFieldUpdater is better, and also eliminates the object
> creation in this path.
> Change callCount to long to head off possibility of int overflow.
> 
> Delete four lines above, change line below to
>     long currentCount = callCountUpdater.incrementAndGet(this);
> 
> See other comment, above.
> 
> Web research, performance of using AtomicFieldUpdater should be similar to
that
> of AtomicInteger. Creation is expensive, but it's one time only, a static
final.

Done.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:227: if (currentCount
== threshold) {
On 2017/08/15 21:00:03, andy.heninger wrote:
> This can be called with varying threshold values. Should the check for
(compiled
> != null) come first, in case some earlier call with a smaller threshold
already
> caused the compile to happen?

The threshold never changes. I re-wrote the code to save it in the macros
instead of being passed as an argument, which I believe is more readable.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/Worker1.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:143: // TODO: Make sure this is
thread safe.
On 2017/08/15 21:00:04, andy.heninger wrote:
> Looks suspicious to me. multiplier comes straight out of the Macro props. The
> chain link, the rest is constant.

I went ahead and modified the QuantityChain implementations to all enforce
safety with regard to the quantity chain (storing the parent in a final field). 
This should make thread safety more self-apparent, but it makes the code a
little bit less readable.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:179: MurkyModifier murkyMod = new
MurkyModifier(false);
On 2017/08/15 21:00:04, andy.heninger wrote:
> Yikes, names. including what does 'false' mean, when out of context.

Acknowledged. Added a comment.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted