Hi Naoto,
Historical indeed, and true, it was not exposed prior to JDK6. Just need
to make sure other classes within the package won't accidentally use its
set method or mark it if it's 'deprecated'. But it's a minor issue. I'm
fine with your changeset as is.
Best,
Joe
On 1/3/20 1:24 PM, naoto.s...@oracle.com wrote:
Hi Joe,
Thanks again for the review. The reason the way it is is all
historical. percent/perMill/minusSign all had public APIs for the
'char' version since inception, and text version APIs were added later
(JDK13). Thus they had to be in sync (both fields are accessible
through API). On the other hand, exponential was private till JDK6,
and at that time I guess the engineer decided only to expose public
access to its text version, i.e., effectively deprecate 'char' version
interface and field. I guess that's why he/she did not bother make
them in sync, IMO. So there seems to be no explicit reason (to be
noted in the source code) for not syncing.
My $.02
Naoto
On 1/3/20 11:40 AM, Joe Wang wrote:
Hi Naoto,
The change looks fine to me as only monetaryGroupingSeparator was
added to equals.
I can't help to note though that, all fields participated in the
equals calculation except exponential. Some of the other fields are
in similar situations (one is public and the other not), e.g. percent
and percentText, perMill and perMillText, minusSign and
minusSignText, and also the currency related fields, but they all are
included. It looks like exponential was never publicly accessible,
but the (1.6) added exponentialSeparator became public. It's probably
not necessary to include all of them in the first place as they are
in sync. that is, changing one would change the other -- and in this
regard, exponential is an exception: setExponentialSymbol won't
change exponential.
I understand this is all historical and it doesn't affect your
changeset. If the reason is known, it won't hurt to add some notes as
the other setXxxText clearly stated the relationship with their
non-Text representation. If not, it's fine to me to not have to spend
the extra time.
Best,
Joe
On 1/3/20 9:23 AM, naoto.s...@oracle.com wrote:
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