Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
Thanks for the update. Looks fine.
I have no more comments.
Roger
On 12/04/2018 10:50 AM, Nishit Jain wrote:
Thanks Roger,
Updated Webrev:
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/
Regards,
Nishit Jain
On 04-12-2018 00:58, Roger Riggs wrote:
Hi Nishit,
Thanks for the updates and cleanup.
CompactNumberFormat.java:
- 827: To locate a single character use:
if (pattern.indexOf(QUOTE) < 0) { ... }
OK.
- 1488: Since infinite returns do not depend on any of the code
after line 1454,
the 1488- 1494 could be moved to 1454. (It is correct where it is).
The computeParseMultiplier decides whether parse string is positive or
negative based on the prefix and suffix, so the
status[STATUS_POSITIVE] can be also be used to return positive or
negative infinity.
ok
Other minor changes in parse():
- Taken out "int numPosition" (line 1444 in webrev.05) and used
earlier defined variable "position" instead, as "position" previous
value is not used after that statement.
- Moved the variable "Number multiplier;" (line 1447 in webrev.05) to
the place where it is assigned the value.
- Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.
- in API design, I would have put the position argument immediately
after text since the position
is closely related to the text argument (in matchAffix and
matchPrefixAndSuffix methods).
Its probably not worth the change in these private methods.
Yes, it is better to move it next to "text". Updated both "matchAffix"
and "matchPrefixAndSuffix" methods.
Regards,
Nishit Jain
comments below...
On 12/03/2018 07:22 AM, Nishit Jain wrote:
Thanks Roger,
Updated the webrev based on thebelow suggested changes and some
clean up.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/
On 01-12-2018 01:03, Roger Riggs wrote:
Hi Nishit,
Some comments, a couple of possible bugs and several performance
related issues
could be deferred. Both formatting and parsing seem to be quite
heavyweight
due to looping and combinatorics.
CompactNumberFormat.java
661, 727, 1507: Is there a reason to convert the divisor to a
string and then re-parse it to create a new BigDecimal?
(IntelliJ observes that divide is called without a rounding
argument.)
Given that the divisors are all powers of 10 and the digit
list is a base 10 set of digits
it seems more efficient to just move the decimal point then
to do the math.
BTW, the divisor.toString() is preferred over concat with
"". (looks like a hack).
It would be more efficient to write two methods that would
pass the Number
and return a BigInteger or BigDecimal by dispatching on the
concrete type and
using the appropriate constructor.
Changed concatenation with toString() and added a rounding
parameter, but not getting the benefit of having two methods and
returning respective concrete type using constructors.
I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs
BigDecimal).
The code is ok as is.
781: @param prefix - the parameter name is suffix
804: move the int start = inside the if.
826: expandAffix can be more efficient if tests the pattern for
the presence of QUOTE
and returns the pattern if the QUOTE is not present. That
would be the most common case.
914: Reduce the number of compares by reordering to:
if number > currentValue; multiply and continue
if number < currentValue break;
else ==; assign matched index and break;
In the normal case, there will be only one compare in the
loop until it is to exit.
1109: IntelliJ observes that within the case QUOTE, the if (ch ==
QUOTE) is always true
so the if is redundant.
OK.
1205: It looks odd to prepend two characters '- to the prefix. Is
the single quote correct?
Where's the closing quote if so.
It is to specify that the minus sign is a special character, which
needs to be replaced with its localized equivalent at the time of
formatting.
Internally, there is a difference between a "minus sign prefix with
a single quote" and a "minus sign" (it depends on how user specify
the pattern), because the later one is considered as a literal and
should be used exactly same in the formatted output, but the one
prefixed with a single quote is replaced with its localized symbol
using DecimalFormatSymbols.getMinusSign().
thanks for the explanation.
1394: matchedPosPrefix.length() is compared to
negativePrefix.length().
That's an unusual mixing of positive and negative! Please
re-check.
Yes, it was a mistake.
1363: Can there be an early exit from this loop if one of the
prefixes is identified?
The pattern of comparing for a prefix/suffix and the length
might warrant
creating a private method to reduce the repetitive parts of
the code.
I had
Re: RFR 8177552: Compact Number Formatting support
Thanks Roger,
Updated Webrev:
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/
Regards,
Nishit Jain
On 04-12-2018 00:58, Roger Riggs wrote:
Hi Nishit,
Thanks for the updates and cleanup.
CompactNumberFormat.java:
- 827: To locate a single character use:
if (pattern.indexOf(QUOTE) < 0) { ... }
OK.
- 1488: Since infinite returns do not depend on any of the code after
line 1454,
the 1488- 1494 could be moved to 1454. (It is correct where it is).
The computeParseMultiplier decides whether parse string is positive or
negative based on the prefix and suffix, so the status[STATUS_POSITIVE]
can be also be used to return positive or negative infinity.
Other minor changes in parse():
- Taken out "int numPosition" (line 1444 in webrev.05) and used earlier
defined variable "position" instead, as "position" previous value is not
used after that statement.
- Moved the variable "Number multiplier;" (line 1447 in webrev.05) to
the place where it is assigned the value.
- Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.
- in API design, I would have put the position argument immediately
after text since the position
is closely related to the text argument (in matchAffix and
matchPrefixAndSuffix methods).
Its probably not worth the change in these private methods.
Yes, it is better to move it next to "text". Updated both "matchAffix"
and "matchPrefixAndSuffix" methods.
Regards,
Nishit Jain
comments below...
On 12/03/2018 07:22 AM, Nishit Jain wrote:
Thanks Roger,
Updated the webrev based on thebelow suggested changes and some clean
up.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/
On 01-12-2018 01:03, Roger Riggs wrote:
Hi Nishit,
Some comments, a couple of possible bugs and several performance
related issues
could be deferred. Both formatting and parsing seem to be quite
heavyweight
due to looping and combinatorics.
CompactNumberFormat.java
661, 727, 1507: Is there a reason to convert the divisor to a string
and then re-parse it to create a new BigDecimal?
(IntelliJ observes that divide is called without a rounding
argument.)
Given that the divisors are all powers of 10 and the digit
list is a base 10 set of digits
it seems more efficient to just move the decimal point then
to do the math.
BTW, the divisor.toString() is preferred over concat with
"". (looks like a hack).
It would be more efficient to write two methods that would
pass the Number
and return a BigInteger or BigDecimal by dispatching on the
concrete type and
using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter,
but not getting the benefit of having two methods and returning
respective concrete type using constructors.
I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs
BigDecimal).
The code is ok as is.
781: @param prefix - the parameter name is suffix
804: move the int start = inside the if.
826: expandAffix can be more efficient if tests the pattern for the
presence of QUOTE
and returns the pattern if the QUOTE is not present. That
would be the most common case.
914: Reduce the number of compares by reordering to:
if number > currentValue; multiply and continue
if number < currentValue break;
else ==; assign matched index and break;
In the normal case, there will be only one compare in the
loop until it is to exit.
1109: IntelliJ observes that within the case QUOTE, the if (ch ==
QUOTE) is always true
so the if is redundant.
OK.
1205: It looks odd to prepend two characters '- to the prefix. Is
the single quote correct?
Where's the closing quote if so.
It is to specify that the minus sign is a special character, which
needs to be replaced with its localized equivalent at the time of
formatting.
Internally, there is a difference between a "minus sign prefix with a
single quote" and a "minus sign" (it depends on how user specify the
pattern), because the later one is considered as a literal and should
be used exactly same in the formatted output, but the one prefixed
with a single quote is replaced with its localized symbol using
DecimalFormatSymbols.getMinusSign().
thanks for the explanation.
1394: matchedPosPrefix.length() is compared to negativePrefix.length().
That's an unusual mixing of positive and negative! Please
re-check.
Yes, it was a mistake.
1363: Can there be an early exit from this loop if one of the
prefixes is identified?
The pattern of comparing for a prefix/suffix and the length
might warrant
creating a private method to reduce the repetitive parts of
the code.
I had thought about it, but it was difficult unless the whole list of
affixes are traversed, because there is always a chance to get longer
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
Thanks for the updates and cleanup.
CompactNumberFormat.java:
- 827: To locate a single character use:
if (pattern.indexOf(QUOTE) < 0) { ... }
- 1488: Since infinite returns do not depend on any of the code after
line 1454,
the 1488- 1494 could be moved to 1454. (It is correct where it is).
- in API design, I would have put the position argument immediately
after text since the position
is closely related to the text argument (in matchAffix and
matchPrefixAndSuffix methods).
Its probably not worth the change in these private methods.
comments below...
On 12/03/2018 07:22 AM, Nishit Jain wrote:
Thanks Roger,
Updated the webrev based on thebelow suggested changes and some clean up.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/
On 01-12-2018 01:03, Roger Riggs wrote:
Hi Nishit,
Some comments, a couple of possible bugs and several performance
related issues
could be deferred. Both formatting and parsing seem to be quite
heavyweight
due to looping and combinatorics.
CompactNumberFormat.java
661, 727, 1507: Is there a reason to convert the divisor to a string
and then re-parse it to create a new BigDecimal?
(IntelliJ observes that divide is called without a rounding
argument.)
Given that the divisors are all powers of 10 and the digit
list is a base 10 set of digits
it seems more efficient to just move the decimal point then
to do the math.
BTW, the divisor.toString() is preferred over concat with
"". (looks like a hack).
It would be more efficient to write two methods that would
pass the Number
and return a BigInteger or BigDecimal by dispatching on the
concrete type and
using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter,
but not getting the benefit of having two methods and returning
respective concrete type using constructors.
I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs
BigDecimal).
The code is ok as is.
781: @param prefix - the parameter name is suffix
804: move the int start = inside the if.
826: expandAffix can be more efficient if tests the pattern for the
presence of QUOTE
and returns the pattern if the QUOTE is not present. That
would be the most common case.
914: Reduce the number of compares by reordering to:
if number > currentValue; multiply and continue
if number < currentValue break;
else ==; assign matched index and break;
In the normal case, there will be only one compare in the
loop until it is to exit.
1109: IntelliJ observes that within the case QUOTE, the if (ch ==
QUOTE) is always true
so the if is redundant.
OK.
1205: It looks odd to prepend two characters '- to the prefix. Is
the single quote correct?
Where's the closing quote if so.
It is to specify that the minus sign is a special character, which
needs to be replaced with its localized equivalent at the time of
formatting.
Internally, there is a difference between a "minus sign prefix with a
single quote" and a "minus sign" (it depends on how user specify the
pattern), because the later one is considered as a literal and should
be used exactly same in the formatted output, but the one prefixed
with a single quote is replaced with its localized symbol using
DecimalFormatSymbols.getMinusSign().
thanks for the explanation.
1394: matchedPosPrefix.length() is compared to negativePrefix.length().
That's an unusual mixing of positive and negative! Please
re-check.
Yes, it was a mistake.
1363: Can there be an early exit from this loop if one of the
prefixes is identified?
The pattern of comparing for a prefix/suffix and the length
might warrant
creating a private method to reduce the repetitive parts of the
code.
I had thought about it, but it was difficult unless the whole list of
affixes are traversed, because there is always a chance to get longer
affix later in the list. I thought to sort the lists based on the
length and do the match, but in that case indexes get disordered
across lists (divisor, prefix, suffix, compact patterns), and
computation will become more complicated.
Updated the code by moving the repetitive parts in the loop to private
methods.
Nice use of an private method to avoid code replication.
1593: extra space between "- ("; should be the same style as 1591
1627, 1363: Is an early exit from this loop possible?
or a quick comparison to avoid the regionMatches.
Do the regionMatches *only if* the prefix/suffix is longer
than the suffix already compared?
Yes, I think this can be done. Updated.
+1
1721: IntelliJ observes that if (gotNeg) is redundant since 1708
rules out them being both true or both false.
Updated
Thanks, Roger
Regards,
Nishit Jain
Thanks, Roger
On 11/28/18 3:46
Re: RFR 8177552: Compact Number Formatting support
Thanks Roger,
Updated the webrev based on thebelow suggested changes and some clean up.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/
On 01-12-2018 01:03, Roger Riggs wrote:
Hi Nishit,
Some comments, a couple of possible bugs and several performance
related issues
could be deferred. Both formatting and parsing seem to be quite
heavyweight
due to looping and combinatorics.
CompactNumberFormat.java
661, 727, 1507: Is there a reason to convert the divisor to a string
and then re-parse it to create a new BigDecimal?
(IntelliJ observes that divide is called without a rounding
argument.)
Given that the divisors are all powers of 10 and the digit
list is a base 10 set of digits
it seems more efficient to just move the decimal point then to
do the math.
BTW, the divisor.toString() is preferred over concat with "".
(looks like a hack).
It would be more efficient to write two methods that would
pass the Number
and return a BigInteger or BigDecimal by dispatching on the
concrete type and
using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter,
but not getting the benefit of having two methods and returning
respective concrete type using constructors.
I didn't get the point of having two methods. Can you please elaborate?
781: @param prefix - the parameter name is suffix
804: move the int start = inside the if.
826: expandAffix can be more efficient if tests the pattern for the
presence of QUOTE
and returns the pattern if the QUOTE is not present. That
would be the most common case.
914: Reduce the number of compares by reordering to:
if number > currentValue; multiply and continue
if number < currentValue break;
else ==; assign matched index and break;
In the normal case, there will be only one compare in the loop
until it is to exit.
1109: IntelliJ observes that within the case QUOTE, the if (ch ==
QUOTE) is always true
so the if is redundant.
OK.
1205: It looks odd to prepend two characters '- to the prefix. Is the
single quote correct?
Where's the closing quote if so.
It is to specify that the minus sign is a special character, which needs
to be replaced with its localized equivalent at the time of formatting.
Internally, there is a difference between a "minus sign prefix with a
single quote" and a "minus sign" (it depends on how user specify the
pattern), because the later one is considered as a literal and should be
used exactly same in the formatted output, but the one prefixed with a
single quote is replaced with its localized symbol using
DecimalFormatSymbols.getMinusSign().
1394: matchedPosPrefix.length() is compared to negativePrefix.length().
That's an unusual mixing of positive and negative! Please re-check.
Yes, it was a mistake.
1363: Can there be an early exit from this loop if one of the
prefixes is identified?
The pattern of comparing for a prefix/suffix and the length
might warrant
creating a private method to reduce the repetitive parts of the
code.
I had thought about it, but it was difficult unless the whole list of
affixes are traversed, because there is always a chance to get longer
affix later in the list. I thought to sort the lists based on the length
and do the match, but in that case indexes get disordered across lists
(divisor, prefix, suffix, compact patterns), and computation will become
more complicated.
Updated the code by moving the repetitive parts in the loop to private
methods.
1593: extra space between "- ("; should be the same style as 1591
1627, 1363: Is an early exit from this loop possible?
or a quick comparison to avoid the regionMatches.
Do the regionMatches *only if* the prefix/suffix is longer than
the suffix already compared?
Yes, I think this can be done. Updated.
1721: IntelliJ observes that if (gotNeg) is redundant since 1708
rules out them being both true or both false.
Updated
Regards,
Nishit Jain
Thanks, Roger
On 11/28/18 3:46 AM, Nishit Jain wrote:
Thanks Naoto,
Updated.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
Some comments, a couple of possible bugs and several performance
related issues
could be deferred. Both formatting and parsing seem to be quite heavyweight
due to looping and combinatorics.
CompactNumberFormat.java
661, 727, 1507: Is there a reason to convert the divisor to a string and
then re-parse it to create a new BigDecimal?
(IntelliJ observes that divide is called without a rounding
argument.)
Given that the divisors are all powers of 10 and the digit list
is a base 10 set of digits
it seems more efficient to just move the decimal point then to
do the math.
BTW, the divisor.toString() is preferred over concat with "".
(looks like a hack).
It would be more efficient to write two methods that would pass
the Number
and return a BigInteger or BigDecimal by dispatching on the
concrete type and
using the appropriate constructor.
781: @param prefix - the parameter name is suffix
804: move the int start = inside the if.
826: expandAffix can be more efficient if tests the pattern for the
presence of QUOTE
and returns the pattern if the QUOTE is not present. That
would be the most common case.
914: Reduce the number of compares by reordering to:
if number > currentValue; multiply and continue
if number < currentValue break;
else ==; assign matched index and break;
In the normal case, there will be only one compare in the loop
until it is to exit.
1109: IntelliJ observes that within the case QUOTE, the if (ch ==
QUOTE) is always true
so the if is redundant.
1205: It looks odd to prepend two characters '- to the prefix. Is the
single quote correct?
Where's the closing quote if so.
1394: matchedPosPrefix.length() is compared to negativePrefix.length().
That's an unusual mixing of positive and negative! Please re-check.
1363: Can there be an early exit from this loop if one of the prefixes
is identified?
The pattern of comparing for a prefix/suffix and the length might
warrant
creating a private method to reduce the repetitive parts of the code.
1593: extra space between "- ("; should be the same style as 1591
1627, 1363: Is an early exit from this loop possible?
or a quick comparison to avoid the regionMatches.
Do the regionMatches *only if* the prefix/suffix is longer than
the suffix already compared?
1721: IntelliJ observes that if (gotNeg) is redundant since 1708 rules
out them being both true or both false.
Thanks, Roger
On 11/28/18 3:46 AM, Nishit Jain wrote:
Thanks Naoto,
Updated.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/
Re: RFR 8177552: Compact Number Formatting support
Looks good to me.
Naoto
On 11/28/18 3:46 AM, Nishit Jain wrote:
Thanks Naoto,
Updated.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/
Regards,
Nishit Jain
On 27-11-2018 20:55, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 11:11 PM, Nishit Jain wrote:
Hi Naoto,
On 26-11-2018 21:01, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also
says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method
that creates a scientific notation format. In a pattern, the
exponent character immediately followed by one or more digit
characters indicates scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their
pattern string.
Anyway, my point is that if you prefer to treat the scientific
notation differently between DecimalFormat and CompactDecimalFormat,
then it will need to be clarified in the spec. Personally I agree
that it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the
exponential numbers, I have added a statement in the parse() API.
*/"CompactNumberFormat parse does not allow parsing exponential
number strings. For example, parsing a string "1.05E4K" in US locale
breaks at character 'E' and returns 1.05."/*
That's better. I'd replace "exponential number strings" with
"scientific notations." You may want to check the final wording with
English natives.
Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/
Will also update the CSR and refinalize it.
Thanks.
Naoto
Regards,
Nishit Jain
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special
pattern characters, but currently the implementation only cares for
the minus sign. Should other localizable pattern chars be taken
care of, such as percent sign?
- Other special characters like '%' percent sign are not allowed as
per CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue
like JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of
number format like "1.05E4K" should not be expected from
CompactNumberFormat, I am even doubtful that such forms
("1.05E4K") are used anywhere where exponential and compact form
are together used. If formatting and parsing of exponential
numbers are needed it should be done by DecimalFormat scientific
instance *not *with the general number instance.So, I don't think
that we should allow parsing of exponential numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the
CNF.parse(), to reduce code duplication.
I presume CNF is calling package-private methods in DF to share
the same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this
aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Some more
Re: RFR 8177552: Compact Number Formatting support
Thanks Naoto,
Updated.
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/
Regards,
Nishit Jain
On 27-11-2018 20:55, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 11:11 PM, Nishit Jain wrote:
Hi Naoto,
On 26-11-2018 21:01, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also
says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method
that creates a scientific notation format. In a pattern, the
exponent character immediately followed by one or more digit
characters indicates scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their
pattern string.
Anyway, my point is that if you prefer to treat the scientific
notation differently between DecimalFormat and CompactDecimalFormat,
then it will need to be clarified in the spec. Personally I agree
that it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the
exponential numbers, I have added a statement in the parse() API.
*/"CompactNumberFormat parse does not allow parsing exponential
number strings. For example, parsing a string "1.05E4K" in US locale
breaks at character 'E' and returns 1.05."/*
That's better. I'd replace "exponential number strings" with
"scientific notations." You may want to check the final wording with
English natives.
Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/
Will also update the CSR and refinalize it.
Thanks.
Naoto
Regards,
Nishit Jain
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special
pattern characters, but currently the implementation only cares for
the minus sign. Should other localizable pattern chars be taken
care of, such as percent sign?
- Other special characters like '%' percent sign are not allowed as
per CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue
like JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of
number format like "1.05E4K" should not be expected from
CompactNumberFormat, I am even doubtful that such forms
("1.05E4K") are used anywhere where exponential and compact form
are together used. If formatting and parsing of exponential
numbers are needed it should be done by DecimalFormat scientific
instance *not *with the general number instance.So, I don't think
that we should allow parsing of exponential numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the
CNF.parse(), to reduce code duplication.
I presume CNF is calling package-private methods in DF to share
the same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this
aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix()
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
On 11/26/18 11:11 PM, Nishit Jain wrote:
Hi Naoto,
On 26-11-2018 21:01, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says
that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method
that creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters
indicates scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their
pattern string.
Anyway, my point is that if you prefer to treat the scientific
notation differently between DecimalFormat and CompactDecimalFormat,
then it will need to be clarified in the spec. Personally I agree that
it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the
exponential numbers, I have added a statement in the parse() API.
*/"CompactNumberFormat parse does not allow parsing exponential number
strings. For example, parsing a string "1.05E4K" in US locale breaks at
character 'E' and returns 1.05."/*
That's better. I'd replace "exponential number strings" with "scientific
notations." You may want to check the final wording with English natives.
Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/
Will also update the CSR and refinalize it.
Thanks.
Naoto
Regards,
Nishit Jain
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
- Other special characters like '%' percent sign are not allowed as
per CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from
CompactNumberFormat, I am even doubtful that such forms ("1.05E4K")
are used anywhere where exponential and compact form are together
used. If formatting and parsing of exponential numbers are needed it
should be done by DecimalFormat scientific instance *not *with the
general number instance.So, I don't think that we should allow
parsing of exponential numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the
CNF.parse(), to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this
aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the
minus sign. Should other localizable pattern chars be
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
On 26-11-2018 21:01, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says
that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method
that creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters
indicates scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their
pattern string.
Anyway, my point is that if you prefer to treat the scientific
notation differently between DecimalFormat and CompactDecimalFormat,
then it will need to be clarified in the spec. Personally I agree that
it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the
exponential numbers, I have added a statement in the parse() API.
*/"CompactNumberFormat parse does not allow parsing exponential number
strings. For example, parsing a string "1.05E4K" in US locale breaks at
character 'E' and returns 1.05."/*
Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/
Will also update the CSR and refinalize it.
Regards,
Nishit Jain
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
- Other special characters like '%' percent sign are not allowed as
per CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from
CompactNumberFormat, I am even doubtful that such forms ("1.05E4K")
are used anywhere where exponential and compact form are together
used. If formatting and parsing of exponential numbers are needed it
should be done by DecimalFormat scientific instance *not *with the
general number instance.So, I don't think that we should allow
parsing of exponential numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the
CNF.parse(), to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this
aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E'
should be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the
minus sign. Should other localizable pattern chars be taken care
of, such as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit, Thanks for all the updates. The tests looks ok. On 11/26/2018 02:56 AM, Nishit Jain wrote: Hi Roger, Please find my comments belowand check the updated webrev. http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/ Regards, Nishit Jain On 22-11-2018 00:04, Roger Riggs wrote: Hi Nishit, Comments on the tests: - The tests looks to be quite complete. - Have the locale specific data been independently verified? Or are they just assumed to be correct based on using CNF to generate the formatted strings? I have manually verified the data against the compact patterns in the resource bundles to the best I can. In some cases, like testing CNF rounding behavior "TestCNFRounding", the data is also verified against the output produced by DecimalFormat. ok - Is there any overlap between the format and parse patterns that can be removed; using the same dataprovider for both format and parse (and an extra provider for unique cases). Yes, there were patterns overlap in CompactPatternsValidity (now renamed as TestCompactPatternsValidity), patterns are taken out and shared between format and parse. - Using TestNG consistently would improve the test suite. OK. Updated EqualityCheck and SerializationTest (now named as TestEquality and TestSerialization) to use TestNG. - In comments, Capitalize the first word - The names of the test files should be more consistent, some include Test at the beginning, some at the end and some not at all. The utility classes (CompactFormatAndParse) name doesn't make it clear it is not a test itself. Updated. Serialization Test: should be comparing the fields of the Format instances, not only that it formats a value the same. It also compares the equality (if (!fmt.equals(obj))) so fields of the instances are also matched. To setup for future revisions, several serialized CNF instances should be hardcoded in the test and deserialized to be checked against the current CNF instances. Added TestDeserializeCNF.java which deserializes the hardcoded instances in cnf1.ser.txt and cnf2.ser.txt. In the comments, also added the API used to create the hexdump of the serializable instance, please check if that is the correct way. typo: "repspective" There must have been some post processing; the code in serialize() doesn't break the lines at 70 chars. looks fine Using testng dataproviders would show a more regular structure. I do not find the use of data provider to be useful here, as we just have some instances which are serialized and deserialized with no specific data to test. The CNF instances or the formats are the data. Serializing all the cases to a single file makes the debugging harder; but if it never fails... ok as is. CompactFormatAndParse.java: - The method don't need "public" since they are used only in the test. - unused import of BigInteger OK EqualityCheck: - Its good form to always have an @run line, even if for default behavior. Moved it to use TestNG and added corresponding @run line - The CNF.equals method includes both symbols and decimal pattern; are there tests for those being the different? Thanks. Added. CompactPatternsValidity.java: -60: Indentation of continued data array values would make it more readable. OK - Is there any overlap between the format and parse patterns that can be removed? Using the same dataprovider for both format and parse (and an extra provider for unique cases). Yes, modified CompactPatternsValidity.java, as mentioned in the above comment CNFRoundingTest.java: - Can the Rounding mode test methods be consolidated and pass in the desired rounding mode. It would save on some boilerplate. Yes, updated. Looks good, Just an observation, no change needed, but since CNF does not have a toString() method, I expected to see a test method to print the values of a CNF as a debugging aid. If there is any mismatch, it just prints the identity of the CNF's but no information about what the fields are. Thanks, Roger Regards, Nishit Jain Thanks, Roger On 11/21/2018 03:53 AM, Nishit Jain wrote: Hi Naoto, Updated the webrev based on suggestions http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/ Changes made: - Replaced List with String[] to be added to the the resource bundles - refactored DecimalFormat.subparse() to be used by the CNF.parse(), to reduce code duplication. - Also updated it with other changes as suggested in the comments Regards, Nishit Jain On 20-11-2018 00:33, naoto.s...@oracle.com wrote: Hi Nishit, On 11/18/18 10:29 PM, Nishit Jain wrote: Hi Naoto, Please check my comments inline. On 17-11-2018 04:52, naoto.s...@oracle.com wrote: Hi Nishit, Here are my comments: - CLDRConverter: As the compact pattern no more employs List, can we eliminate stringListEntry/Element, and use Array equivalent instead? Since the CNF design does not put any limit on the size of compact pattern, so at
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
On 11/26/18 12:41 AM, Nishit Jain wrote:
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method that
creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters indicates
scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
I am not sure the quoted sentence should be interpreted that way. My
understanding is that the section means there is no public
NumberFormat.getScientificInstance() method (cf. line 601 at
NumberFormat.java), so that users will have to use 'E' in their pattern
string.
Anyway, my point is that if you prefer to treat the scientific notation
differently between DecimalFormat and CompactDecimalFormat, then it will
need to be clarified in the spec. Personally I agree that it is not
practical to interpret E in the CNF.
Naoto
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
- Other special characters like '%' percent sign are not allowed as per
CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be shared
with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from CompactNumberFormat,
I am even doubtful that such forms ("1.05E4K") are used anywhere where
exponential and compact form are together used. If formatting and
parsing of exponential numbers are needed it should be done by
DecimalFormat scientific instance *not *with the general number
instance.So, I don't think that we should allow parsing of exponential
numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
To add to my previous mail comment, the DecimalFormat spec also says that
/*"DecimalFormat can be instructed to format and parse scientific
notation only via a pattern; there is currently no factory method that
creates a scientific notation format. In a pattern, the exponent
character immediately followed by one or more digit characters indicates
scientific notation. "
*/That is, exponent formatting and parsing is instructed only via a
scientific notation pattern and I think should not be there with
*general number* formatting.
Updated webrev based on the other comments
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/
> Some more comments (all in CompactNumberFormat.java)
> line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
- Other special characters like '%' percent sign are not allowed as per
CNF compact pattern spec
> line 869, 888: Define what -1 means as a ret value.
- OK.
> line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be shared
with other locations that use "10" for arithmetic operation.
- OK, made it static final and renamed it as RANGE_MULTIPLIER
> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like
JDK-8211161, which we know is not an issue.
Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from CompactNumberFormat,
I am even doubtful that such forms ("1.05E4K") are used anywhere where
exponential and compact form are together used. If formatting and
parsing of exponential numbers are needed it should be done by
DecimalFormat scientific instance *not *with the general number
instance.So, I don't think that we should allow parsing of exponential
numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the
resource bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such
as percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be
shared with other locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser,
it becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better
to keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another
bundle format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of
Re: RFR 8177552: Compact Number Formatting support
Hi Roger, Please find my comments belowand check the updated webrev. http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/ Regards, Nishit Jain On 22-11-2018 00:04, Roger Riggs wrote: Hi Nishit, Comments on the tests: - The tests looks to be quite complete. - Have the locale specific data been independently verified? Or are they just assumed to be correct based on using CNF to generate the formatted strings? I have manually verified the data against the compact patterns in the resource bundles to the best I can. In some cases, like testing CNF rounding behavior "TestCNFRounding", the data is also verified against the output produced by DecimalFormat. - Is there any overlap between the format and parse patterns that can be removed; using the same dataprovider for both format and parse (and an extra provider for unique cases). Yes, there were patterns overlap in CompactPatternsValidity (now renamed as TestCompactPatternsValidity), patterns are taken out and shared between format and parse. - Using TestNG consistently would improve the test suite. OK. Updated EqualityCheck and SerializationTest (now named as TestEquality and TestSerialization) to use TestNG. - In comments, Capitalize the first word - The names of the test files should be more consistent, some include Test at the beginning, some at the end and some not at all. The utility classes (CompactFormatAndParse) name doesn't make it clear it is not a test itself. Updated. Serialization Test: should be comparing the fields of the Format instances, not only that it formats a value the same. It also compares the equality (if (!fmt.equals(obj))) so fields of the instances are also matched. To setup for future revisions, several serialized CNF instances should be hardcoded in the test and deserialized to be checked against the current CNF instances. Added TestDeserializeCNF.java which deserializes the hardcoded instances in cnf1.ser.txt and cnf2.ser.txt. In the comments, also added the API used to create the hexdump of the serializable instance, please check if that is the correct way. Using testng dataproviders would show a more regular structure. I do not find the use of data provider to be useful here, as we just have some instances which are serialized and deserialized with no specific data to test. CompactFormatAndParse.java: - The method don't need "public" since they are used only in the test. - unused import of BigInteger OK EqualityCheck: - Its good form to always have an @run line, even if for default behavior. Moved it to use TestNG and added corresponding @run line - The CNF.equals method includes both symbols and decimal pattern; are there tests for those being the different? Thanks. Added. CompactPatternsValidity.java: -60: Indentation of continued data array values would make it more readable. OK - Is there any overlap between the format and parse patterns that can be removed? Using the same dataprovider for both format and parse (and an extra provider for unique cases). Yes, modified CompactPatternsValidity.java, as mentioned in the above comment CNFRoundingTest.java: - Can the Rounding mode test methods be consolidated and pass in the desired rounding mode. It would save on some boilerplate. Yes, updated. Regards, Nishit Jain Thanks, Roger On 11/21/2018 03:53 AM, Nishit Jain wrote: Hi Naoto, Updated the webrev based on suggestions http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/ Changes made: - Replaced List with String[] to be added to the the resource bundles - refactored DecimalFormat.subparse() to be used by the CNF.parse(), to reduce code duplication. - Also updated it with other changes as suggested in the comments Regards, Nishit Jain On 20-11-2018 00:33, naoto.s...@oracle.com wrote: Hi Nishit, On 11/18/18 10:29 PM, Nishit Jain wrote: Hi Naoto, Please check my comments inline. On 17-11-2018 04:52, naoto.s...@oracle.com wrote: Hi Nishit, Here are my comments: - CLDRConverter: As the compact pattern no more employs List, can we eliminate stringListEntry/Element, and use Array equivalent instead? Since the CNF design does not put any limit on the size of compact pattern, so at the time of parsing the CLDR xmls using SAX parser, it becomes difficult to identify the size of array when the parent element of compact pattern is encountered, so I think it is better to keep the List while extracting the resources. OK. However I'd not keep the List format on generating the resource bundle, as there is no reason to introduce yet another bundle format other than the existing array of String. - CompactNumberFormat.java Multiple locations: Use StringBuilder instead of StringBuffer. OK line 268: The link points to NumberFormat.getNumberInstance(Locale) instead of DecimalFormat OK. Changed it at line 165 also. line 855: no need to do toString(). length() can detect whether it's empty
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
> I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Personally I don't think that the exponential parsing should be
supported by CompactNumberFormat, because the objective of compact
numbers is to represent numbers in short form. So, parsing of number
format like "1.05E4K" should not be expected from CompactNumberFormat, I
am even doubtful that such forms ("1.05E4K") are used anywhere where
exponential and compact form are together used. If formatting and
parsing of exponential numbers are needed it should be done by
DecimalFormat scientific instance *not *with the general number
instance.So, I don't think that we should allow parsing of exponential
numbers.Comments welcome.
Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the resource
bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
I presume CNF is calling package-private methods in DF to share the
same code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should
be treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a
constant. And it could be statically defined in the class to be shared
with other locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser,
it becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better
to keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another
bundle format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to
NumberFormat.getNumberInstance(Locale) instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether
it's empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer
specializing in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly
is handled, DecimalFormat.parse()/subparse() behaviour is
unpredictable with parseIntegeronly = true when multipliers are
involved (Please see JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should
*not *parse the exponential notation e.g. while parsing "1.05E4K"
the parsing should break at 'E' and returns 1.05, because 'E'
should be considered as unparseable character for general number
format pattern or compact number pattern, but this is not the case
with DecimalFormat.parse(). The below DecimalFormat general number
format instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour
is there with other DecimalFormat instances also e.g. currency
instance.
Do you think this is an issue with DecimalFormat.parse() and CNF
should avoid parsing exponential numbers? Or, should CNF.parse() be
modified to be consistent with DecimalFormat.parse() in this aspect?
No, I understand there are differences. But I see a lot of
duplicated piece
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
On 11/21/18 12:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the resource
bundles
Good.
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to
reduce code duplication.
I presume CNF is calling package-private methods in DF to share the same
code. Some comments noting the sharing would be helpful.
- Also updated it with other changes as suggested in the comments
Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?
I think DecimalFormat and CNF should behave the same, ie. 'E' should be
treated as the exponent without a quote.
Some more comments (all in CompactNumberFormat.java)
line 807: expandAffix() seems to treat localizable special pattern
characters, but currently the implementation only cares for the minus
sign. Should other localizable pattern chars be taken care of, such as
percent sign?
line 869, 888: Define what -1 means as a ret value.
line 897: iterMultiplier be better all capitalized as it is a constant.
And it could be statically defined in the class to be shared with other
locations that use "10" for arithmetic operation.
line 1531: Any possibility this could lead to divide-by-zero?
Naoto
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser, it
becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better to
keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another bundle
format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to NumberFormat.getNumberInstance(Locale)
instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether it's
empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer
specializing in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly
is handled, DecimalFormat.parse()/subparse() behaviour is
unpredictable with parseIntegeronly = true when multipliers are
involved (Please see JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should *not
*parse the exponential notation e.g. while parsing "1.05E4K" the
parsing should break at 'E' and returns 1.05, because 'E' should be
considered as unparseable character for general number format pattern
or compact number pattern, but this is not the case with
DecimalFormat.parse(). The below DecimalFormat general number format
instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour
is there with other DecimalFormat instances also e.g. currency instance.
Do you think this is an issue with DecimalFormat.parse() and CNF
should avoid parsing exponential numbers? Or, should CNF.parse() be
modified to be consistent with DecimalFormat.parse() in this aspect?
No, I understand there are differences. But I see a lot of duplicated
piece of code which I would like to eliminate.
line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls
super. No need to override them.
Since setters are overridden, I think that it is better to override
getters also (even if they are just calling super and have same
javadoc) to keep them at same level. But, if you see no point in
keeping them in CNF, I will remove them. Does that need CSR change?
I don't see any point for override. I don't think there needs a CSR,
but better ask Joe about it.
line 2231: You need to test the type before cast. Otherwise
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() !=
obj.getClass(), so I think there is no need to check the type here.
OK.
Naoto
Regards,
Nishit Jain
Naoto
On 11/16/18 9:54 AM, Nishit Jain wrote:
Hi,
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
Comments on the tests:
- The tests looks to be quite complete.
- Have the locale specific data been independently verified?
Or are they just assumed to be correct based on using CNF to
generate the formatted strings?
- Is there any overlap between the format and parse patterns that can be
removed;
using the same dataprovider for both format and parse (and an extra
provider for unique cases).
- Using TestNG consistently would improve the test suite.
- In comments, Capitalize the first word
- The names of the test files should be more consistent, some include
Test at the beginning,
some at the end and some not at all. The utility classes
(CompactFormatAndParse) name
doesn't make it clear it is not a test itself.
Serialization Test: should be comparing the fields of the Format instances,
not only that it formats a value the same.
To setup for future revisions, several serialized CNF instances should
be hardcoded
in the test and deserialized to be checked against the current CNF
instances.
Using testng dataproviders would show a more regular structure.
CompactFormatAndParse.java:
- The method don't need "public" since they are used only in the test.
- unused import of BigInteger
EqualityCheck:
- Its good form to always have an @run line, even if for default behavior.
- The CNF.equals method includes both symbols and decimal pattern;
are there tests for those being the different?
CompactPatternsValidity.java:
-60: Indentation of continued data array values would make it more
readable.
- Is there any overlap between the format and parse patterns that can
be removed?
Using the same dataprovider for both format and parse (and an extra
provider for unique cases).
CNFRoundingTest.java:
- Can the Rounding mode test methods be consolidated and pass in the
desired rounding mode.
It would save on some boilerplate.
Thanks, Roger
On 11/21/2018 03:53 AM, Nishit Jain wrote:
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the resource
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(),
to reduce code duplication.
- Also updated it with other changes as suggested in the comments
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser,
it becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better
to keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another
bundle format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to NumberFormat.getNumberInstance(Locale)
instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether
it's empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer
specializing in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly
is handled, DecimalFormat.parse()/subparse() behaviour is
unpredictable with parseIntegeronly = true when multipliers are
involved (Please see JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should
*not *parse the exponential notation e.g. while parsing "1.05E4K"
the parsing should break at 'E' and returns 1.05, because 'E' should
be considered as unparseable character for general number format
pattern or compact number pattern, but this is not the case with
DecimalFormat.parse(). The below DecimalFormat general number format
instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour
is there with other DecimalFormat instances also e.g. currency
instance.
Do you think this is an issue with DecimalFormat.parse() and CNF
should avoid parsing exponential numbers? Or, should CNF.parse() be
modified to be consistent with DecimalFormat.parse() in this aspect?
No, I understand there are differences. But I see a lot of duplicated
piece of code which I would like to eliminate.
line
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
Updated the webrev based on suggestions
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/
Changes made:
- Replaced List with String[] to be added to the the resource
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to
reduce code duplication.
- Also updated it with other changes as suggested in the comments
Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs
List, can we eliminate stringListEntry/Element, and use
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser, it
becomes difficult to identify the size of array when the parent
element of compact pattern is encountered, so I think it is better to
keep the List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another bundle
format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to NumberFormat.getNumberInstance(Locale)
instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether it's
empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer
specializing in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly
is handled, DecimalFormat.parse()/subparse() behaviour is
unpredictable with parseIntegeronly = true when multipliers are
involved (Please see JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should *not
*parse the exponential notation e.g. while parsing "1.05E4K" the
parsing should break at 'E' and returns 1.05, because 'E' should be
considered as unparseable character for general number format pattern
or compact number pattern, but this is not the case with
DecimalFormat.parse(). The below DecimalFormat general number format
instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour
is there with other DecimalFormat instances also e.g. currency instance.
Do you think this is an issue with DecimalFormat.parse() and CNF
should avoid parsing exponential numbers? Or, should CNF.parse() be
modified to be consistent with DecimalFormat.parse() in this aspect?
No, I understand there are differences. But I see a lot of duplicated
piece of code which I would like to eliminate.
line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls
super. No need to override them.
Since setters are overridden, I think that it is better to override
getters also (even if they are just calling super and have same
javadoc) to keep them at same level. But, if you see no point in
keeping them in CNF, I will remove them. Does that need CSR change?
I don't see any point for override. I don't think there needs a CSR,
but better ask Joe about it.
line 2231: You need to test the type before cast. Otherwise
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() !=
obj.getClass(), so I think there is no need to check the type here.
OK.
Naoto
Regards,
Nishit Jain
Naoto
On 11/16/18 9:54 AM, Nishit Jain wrote:
Hi,
Please review this non trivial feature addition to NumberFormat API.
The existing NumberFormat API provides locale based support for
formatting and parsing numbers which includes formatting decimal,
percent, currency etc, but the support for formatting a number into
a human readable or compact form is missing. This RFE adds that
feature to format a decimal number in a compact format (e.g. 1000
-> 1K, 100 -> 1M in en_US locale) , which is useful for the
environment where display space is limited, so that the formatted
string can be displayed in that limited space. It is defined by
LDML's specification for Compact Number Formats.
http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats
RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev:
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
Request to please help review the the change.
Regards,
Nishit Jain
Re: RFR 8177552: Compact Number Formatting support
Hi Stephen, Thanks for the comments. The NumberFormat.Style class just defines two styles and currently it is only used by CompactNumberFormat, making it a top level class would require it to be more general purpose and increases the scope. In which case evolving it at a later point of time would bea risk. The current inner NumberFormat.Style has the scope narrow to NF and poses less risk. Regarding providing control of the compact patterns or text, a user can use the CNF constructor and define its own compact patterns/text for that instance, compact NumberFormatProvider can also be used to to get the customized CNF instance. Regards, Nishit Jain On 19-11-2018 19:42, Stephen Colebourne wrote: I'm not a big fan of having a class named `Style` as it is commonly used in business logic. (Yes, its an inner class, but I still think the potential for annoyance is high). java.time.* has `TextStyle`, but I don't think it can be reused here. Maybe the class should be honest and called NumberFormatStyle (as a top level class). More generally, the API does not allow the caller to take control of the text, for example to use "mil" as a suffix for million. eg Think of this method in java.time.* - https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/format/DateTimeFormatterBuilder.html#appendText(java.time.temporal.TemporalField,java.util.Map) Stephen On Fri, 16 Nov 2018 at 17:56, Nishit Jain wrote: Hi, Please review this non trivial feature addition to NumberFormat API. The existing NumberFormat API provides locale based support for formatting and parsing numbers which includes formatting decimal, percent, currency etc, but the support for formatting a number into a human readable or compact form is missing. This RFE adds that feature to format a decimal number in a compact format (e.g. 1000 -> 1K, 100 -> 1M in en_US locale) , which is useful for the environment where display space is limited, so that the formatted string can be displayed in that limited space. It is defined by LDML's specification for Compact Number Formats. http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats RFE: https://bugs.openjdk.java.net/browse/JDK-8177552 Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8188147 Request to please help review the the change. Regards, Nishit Jain
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit,
On 11/18/18 10:29 PM, Nishit Jain wrote:
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs List,
can we eliminate stringListEntry/Element, and use Array equivalent
instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser, it
becomes difficult to identify the size of array when the parent element
of compact pattern is encountered, so I think it is better to keep the
List while extracting the resources.
OK. However I'd not keep the List format on generating the
resource bundle, as there is no reason to introduce yet another bundle
format other than the existing array of String.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to NumberFormat.getNumberInstance(Locale)
instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether it's
empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer specializing
in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly is
handled, DecimalFormat.parse()/subparse() behaviour is unpredictable
with parseIntegeronly = true when multipliers are involved (Please see
JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should *not
*parse the exponential notation e.g. while parsing "1.05E4K" the parsing
should break at 'E' and returns 1.05, because 'E' should be considered
as unparseable character for general number format pattern or compact
number pattern, but this is not the case with DecimalFormat.parse(). The
below DecimalFormat general number format instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour is
there with other DecimalFormat instances also e.g. currency instance.
Do you think this is an issue with DecimalFormat.parse() and CNF should
avoid parsing exponential numbers? Or, should CNF.parse() be modified to
be consistent with DecimalFormat.parse() in this aspect?
No, I understand there are differences. But I see a lot of duplicated
piece of code which I would like to eliminate.
line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls
super. No need to override them.
Since setters are overridden, I think that it is better to override
getters also (even if they are just calling super and have same javadoc)
to keep them at same level. But, if you see no point in keeping them in
CNF, I will remove them. Does that need CSR change?
I don't see any point for override. I don't think there needs a CSR, but
better ask Joe about it.
line 2231: You need to test the type before cast. Otherwise
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() !=
obj.getClass(), so I think there is no need to check the type here.
OK.
Naoto
Regards,
Nishit Jain
Naoto
On 11/16/18 9:54 AM, Nishit Jain wrote:
Hi,
Please review this non trivial feature addition to NumberFormat API.
The existing NumberFormat API provides locale based support for
formatting and parsing numbers which includes formatting decimal,
percent, currency etc, but the support for formatting a number into a
human readable or compact form is missing. This RFE adds that feature
to format a decimal number in a compact format (e.g. 1000 -> 1K,
100 -> 1M in en_US locale) , which is useful for the environment
where display space is limited, so that the formatted string can be
displayed in that limited space. It is defined by LDML's
specification for Compact Number Formats.
http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats
RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
Request to please help review the the change.
Regards,
Nishit Jain
Re: RFR 8177552: Compact Number Formatting support
I'm not a big fan of having a class named `Style` as it is commonly used in business logic. (Yes, its an inner class, but I still think the potential for annoyance is high). java.time.* has `TextStyle`, but I don't think it can be reused here. Maybe the class should be honest and called NumberFormatStyle (as a top level class). More generally, the API does not allow the caller to take control of the text, for example to use "mil" as a suffix for million. eg Think of this method in java.time.* - https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/format/DateTimeFormatterBuilder.html#appendText(java.time.temporal.TemporalField,java.util.Map) Stephen On Fri, 16 Nov 2018 at 17:56, Nishit Jain wrote: > Hi, > > Please review this non trivial feature addition to NumberFormat API. > > The existing NumberFormat API provides locale based support for > formatting and parsing numbers which includes formatting decimal, > percent, currency etc, but the support for formatting a number into a > human readable or compact form is missing. This RFE adds that feature to > format a decimal number in a compact format (e.g. 1000 -> 1K, 100 -> > 1M in en_US locale) , which is useful for the environment where display > space is limited, so that the formatted string can be displayed in that > limited space. It is defined by LDML's specification for Compact Number > Formats. > > http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats > > > RFE: https://bugs.openjdk.java.net/browse/JDK-8177552 > Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/ > CSR: https://bugs.openjdk.java.net/browse/JDK-8188147 > > Request to please help review the the change. > > Regards, > Nishit Jain > > > > >
Re: RFR 8177552: Compact Number Formatting support
Hi Naoto,
Please check my comments inline.
On 17-11-2018 04:52, naoto.s...@oracle.com wrote:
Hi Nishit,
Here are my comments:
- CLDRConverter: As the compact pattern no more employs List,
can we eliminate stringListEntry/Element, and use Array equivalent
instead?
Since the CNF design does not put any limit on the size of compact
pattern, so at the time of parsing the CLDR xmls using SAX parser, it
becomes difficult to identify the size of array when the parent element
of compact pattern is encountered, so I think it is better to keep the
List while extracting the resources.
- CompactNumberFormat.java
Multiple locations: Use StringBuilder instead of StringBuffer.
OK
line 268: The link points to NumberFormat.getNumberInstance(Locale)
instead of DecimalFormat
OK. Changed it at line 165 also.
line 855: no need to do toString(). length() can detect whether it's
empty or not.
line 884: "Overloaded method" reads odd here. I'd prefer specializing
in the "given number" into either long or biginteger.
OK
line 1500: subparseNumber() pretty much shares the same code with
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly is
handled, DecimalFormat.parse()/subparse() behaviour is unpredictable
with parseIntegeronly = true when multipliers are involved (Please see
JDK-8199223).
Also, I had thought that the CNF.parse()/subparseNumber() should *not
*parse the exponential notation e.g. while parsing "1.05E4K" the parsing
should break at 'E' and returns 1.05, because 'E' should be considered
as unparseable character for general number format pattern or compact
number pattern, but this is not the case with DecimalFormat.parse(). The
below DecimalFormat general number format instance
NumberFormat nf = NumberFormat.getNumberInstance();
nf.parse("1.05E4")
Successfully parse the string and returns 10500. The same behaviour is
there with other DecimalFormat instances also e.g. currency instance.
Do you think this is an issue with DecimalFormat.parse() and CNF should
avoid parsing exponential numbers? Or, should CNF.parse() be modified to
be consistent with DecimalFormat.parse() in this aspect?
line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls
super. No need to override them.
Since setters are overridden, I think that it is better to override
getters also (even if they are just calling super and have same javadoc)
to keep them at same level. But, if you see no point in keeping them in
CNF, I will remove them. Does that need CSR change?
line 2231: You need to test the type before cast. Otherwise
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() !=
obj.getClass(), so I think there is no need to check the type here.
Regards,
Nishit Jain
Naoto
On 11/16/18 9:54 AM, Nishit Jain wrote:
Hi,
Please review this non trivial feature addition to NumberFormat API.
The existing NumberFormat API provides locale based support for
formatting and parsing numbers which includes formatting decimal,
percent, currency etc, but the support for formatting a number into a
human readable or compact form is missing. This RFE adds that feature
to format a decimal number in a compact format (e.g. 1000 -> 1K,
100 -> 1M in en_US locale) , which is useful for the environment
where display space is limited, so that the formatted string can be
displayed in that limited space. It is defined by LDML's
specification for Compact Number Formats.
http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats
RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
Request to please help review the the change.
Regards,
Nishit Jain
Re: RFR 8177552: Compact Number Formatting support
Hi Nishit, Here are my comments: - CLDRConverter: As the compact pattern no more employs List, can we eliminate stringListEntry/Element, and use Array equivalent instead? - CompactNumberFormat.java Multiple locations: Use StringBuilder instead of StringBuffer. line 268: The link points to NumberFormat.getNumberInstance(Locale) instead of DecimalFormat line 855: no need to do toString(). length() can detect whether it's empty or not. line 884: "Overloaded method" reads odd here. I'd prefer specializing in the "given number" into either long or biginteger. line 1500: subparseNumber() pretty much shares the same code with DecimalFormat.subparse(). can they be merged? line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls super. No need to override them. line 2231: You need to test the type before cast. Otherwise ClassCastException may be thrown. Naoto On 11/16/18 9:54 AM, Nishit Jain wrote: Hi, Please review this non trivial feature addition to NumberFormat API. The existing NumberFormat API provides locale based support for formatting and parsing numbers which includes formatting decimal, percent, currency etc, but the support for formatting a number into a human readable or compact form is missing. This RFE adds that feature to format a decimal number in a compact format (e.g. 1000 -> 1K, 100 -> 1M in en_US locale) , which is useful for the environment where display space is limited, so that the formatted string can be displayed in that limited space. It is defined by LDML's specification for Compact Number Formats. http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats RFE: https://bugs.openjdk.java.net/browse/JDK-8177552 Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8188147 Request to please help review the the change. Regards, Nishit Jain
RFR 8177552: Compact Number Formatting support
Hi, Please review this non trivial feature addition to NumberFormat API. The existing NumberFormat API provides locale based support for formatting and parsing numbers which includes formatting decimal, percent, currency etc, but the support for formatting a number into a human readable or compact form is missing. This RFE adds that feature to format a decimal number in a compact format (e.g. 1000 -> 1K, 100 -> 1M in en_US locale) , which is useful for the environment where display space is limited, so that the formatted string can be displayed in that limited space. It is defined by LDML's specification for Compact Number Formats. http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats RFE: https://bugs.openjdk.java.net/browse/JDK-8177552 Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8188147 Request to please help review the the change. Regards, Nishit Jain
