This rietveld is my initial attempt at refactoring MeasureFormat to wrap NumberFormatter. Most unit tests ...
6 years, 3 months ago
(2017-12-23 03:55:40 UTC)
#1
This rietveld is my initial attempt at refactoring MeasureFormat to wrap
NumberFormatter. Most unit tests are passing, except for some serialization
ones.
My internal changes were also indirectly affecting MeasureFormat's two
subclasses, CurrencyFormat and TimeUnitFormat. I took the opportunity to also
do some refactoring on those subclasses.
In the middle of the new MeasureUnit, I have a cache-like mechanism going on.
The intent is that NumberFormatter has the greatest performance benefits if you
save an object with all of the settings that you need and only call .format() on
it. I could have saved instances of the three formats that used to be in the
class: formatter, currencyFormatter, and integerFormatter. However, this
approach had two stumbling blocks:
1) The subclass TimeUnitFormat has its own instance of NumberFormat that is
supposed to override the one from the parent class.
2) Due to NumberFormatter's internals, you don't get any performance improvement
if you are formatting two different units back-to-back.
Another alternative, which would be even simpler, would be to have a
single-object cache. This would work great for use cases when you are
formatting the same unit over and over again. However, it wouldn't work for
cases when you are formatting things like "1 year, 2 months, and 30 days".
Addressed first round of comments. Round-trip serialization is still not completely working. I'm not going ...
6 years, 2 months ago
(2018-01-11 05:35:59 UTC)
#9
Addressed first round of comments.
Round-trip serialization is still not completely working. I'm not going to
worry about making backwards-compatible serialization, but I need to make sure
you can deserialize a newly serialized object.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right):
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:175: *
@deprecated ICU 61 This API is ICU internal only.
On 2018/01/10 22:00:13, markus.icu wrote:
> Please add to the API docs what should be used instead.
Done.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:185: */
On 2018/01/10 22:00:13, markus.icu wrote:
> Do package-visible fields show up in the javadoc? If so, please mark them
> @internal.
I'm 95% sure that package-private does not show up in javadoc.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:208: * @param
locale
On 2018/01/10 22:00:13, markus.icu wrote:
> Does this line break come from an auto formatter?
Yes.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:343: // /**
On 2018/01/10 22:00:13, markus.icu wrote:
> Please *delete* code that is not needed any more.
This is a rough draft public-internal API (not even tech preview) that I haven't
ported. Suggestions on what to do with it?
1) Delete it (build error for anyone who depended on it)
2) Make it throw an UnsupportedOperationException
2) Spend time making it work
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:530: //
results[i] = formatMeasure(
On 2018/01/10 22:00:13, markus.icu wrote:
> Please delete obsolete code.
Done.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:677: formatters
= localeToNumericDurationFormatters.get(locale);
On 2018/01/10 22:00:13, markus.icu wrote:
> synchronized?
it looks like SimpleCache is thread-safe. This code comes from the old
getInstance() method higher up in the file, and it wasn't synchronized there.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:698: //
MeasureFormat() {
On 2018/01/10 22:00:13, markus.icu wrote:
> ... delete ...
Done.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:871: //
ImmutableNumberFormat nf = (i == measures.length - 1 ? numberFormat :
integerFormat);
On 2018/01/10 22:00:13, markus.icu wrote:
> delete
Done.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:880: //
results[i] = formatMeasure(measures[i], nf, new StringBuilder(),
fpos).toString();
On 2018/01/10 22:00:13, markus.icu wrote:
> del
Done.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/tests/core/src...
File icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/PluralRangesTest.java
(right):
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/PluralRangesTest.java:72:
// @Test
On 2018/01/10 22:00:13, markus.icu wrote:
> what's going on here?
This is the test for an @internal, not even tech-preview, feature that was
implemented (maybe by Mark?) a few versions ago and never proposed. I didn't
port it in my refactor. I want to propose something similar later this year,
maybe in the 62 timeframe.
https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right): https://codereview.appspot.com/334320043/diff/20001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java#newcode343 icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:343: // /** Ok. Please check that CLDR does not ...
6 years, 2 months ago
(2018-01-11 21:03:25 UTC)
#10
Working on the serialization tonight. https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right): https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java#newcode311 icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:311: // Can't directly use ...
6 years, 2 months ago
(2018-01-13 06:25:21 UTC)
#12
Working on the serialization tonight.
https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java (right):
https://codereview.appspot.com/334320043/diff/80001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/MeasureFormat.java:311: // Can't
directly use the StringBuffer because the rest of the stack expects
StringBuilder
On 2018/01/11 21:03:25, markus.icu wrote:
> I recommend to write implementation code (or even APIs) to write to an
> Appendable, and to catch and re-throw the IOException. See class CaseMap or
> CaseMapImpl for an example.
Done. This required changing a few different methods and moving around the
logic for accounting for field position offset by the StringBuilder's length,
since Appendable doesn't have a .length() method.
Serialization is working, and it's even backwards-compatible thanks to decisions made by previous engineers who've ...
6 years, 2 months ago
(2018-01-13 08:57:46 UTC)
#14
Serialization is working, and it's even backwards-compatible thanks to decisions
made by previous engineers who've worked on this class. To fix the tests, it
turns out all I needed to to was add back a bit of code I had deleted from
CurrencyFormat, and then in the writeReplace methods, call getNumberFormat()
instead of trying to access the numberFormat private field, which is not used by
TimeUnitFormat.
Issue 334320043: ticket:13526 Refactoring MeasureFormat, CurrencyFormat, and TimeUnitFormat to more directly wrap Nu…
(Closed)
Created 6 years, 3 months ago by sffc
Modified 6 years, 1 month ago
Reviewers: markus.icu
Base URL: svn+icussh://source.icu-project.org/repos/icu/trunk/
Comments: 24