Hi Joe,
I revised the changeset, as the cached hash code in DecimalFormatSymbols
needs to be recalculated when any of the relevant fields is mutated.
Here is the updated webrev:
http://cr.openjdk.java.net/~naoto/8227313/webrev.02/
Naoto
On 1/2/20 2:19 PM, Joe Wang wrote:
Happy New Year, Naoto!
Thanks for the explanation and changes. The changeset looks good to me.
-Joe
On 1/2/20 12:50 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Happy new year and thanks for your comments. Please see my replies below:
On 12/23/19 5:20 PM, Joe Wang wrote:
Hi Naoto,
Is there a need for an APINote to note the relationship between the
new get/setMonetaryGroupingSeparator and get/setGroupingSeparator
methods. The new method did state it "May be different from {@code
grouping separator} in some locales", but that may be insufficient.
For example, does setting one method affects the other (seems it
should since the monetaryGroupingSeparator may be initialized by the
groupingSeparator as the impl shows)? If yes, how it's affected?
Setting the custom monetary grouping separator will not affect the
existing normal grouping separator. I added the explanation in the
method description.
If no, is there a compatibility concern? In the current impl, the new
get method is used when "isCurrencyFormat" is true while the new set
method doesn't affect the existing 'groupingSeparator'. For existing
applications that have a custom grouping separator set through
setGroupingSeparator, it seems to me the custom separator won't be
used moving onto the new JDK impl.
Good point. Modified the compatibility risk from minimum to low with
the explanation.
A minor comment about the definition statement in the following:
Add the following new serializable field
in|java.text.DecimalFormatSymbols|class:
|/** * The grouping separator used when formatting currency values. *
* @serial * @since 15 */ private char monetaryGroupingSeparator;|
and that for the new get method:
Gets the character used for thousands separator for currencies.
While the meanings are clear, they were not as consistent as that
between the existing groupingSeparator and its get method, that is:
Character used for thousands separator.
and then the get method states:
Gets the character used for thousands separator.
But this is minor, your call whether to change it or not.
Consistency is important. I replaced all occurrences of "thousands
separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping
separator(s)."
Here are the modified changeset:
http://cr.openjdk.java.net/~naoto/8227313/webrev.01/
as well as the modified CSR at the same URL.
Naoto
Best, and have a great Christmas! :-)
Joe
On 12/20/19 12:57 PM, naoto.s...@oracle.com wrote:
Hi,
Please review the fix for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8227313
The proposed CSR and changeset are located at:
https://bugs.openjdk.java.net/browse/JDK-8235942
https://cr.openjdk.java.net/~naoto/8227313/webrev.00/
The change introduces the new monetary grouping separator in
DecimalFormatSymbols, and it is used if a DecimalFormat instance
designates currency formatting where its pattern includes the
currency symbol (U+00A5). The change makes use of the CLDR data
which provides two distinct grouping separators (one for generic,
the other for currency) in some locales. It also addresses the
similar cases for the decimal separator.
Naoto
