ticket:13285 Adding NumberingSystem constructor methods to DecimalFormatSymbols (J and C).

ticket:13285 Adding NumberingSystem constructor methods to DecimalFormatSymbols (J and C).

[sffc, 2017-07-27 03:11:49]

I have the API proposal ready to send if the change looks good.

[sffc, 2017-07-27 06:15:59]

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
File icu4c/source/test/intltest/tsdcfmsy.cpp (right):

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
icu4c/source/test/intltest/tsdcfmsy.cpp:262: {"my", "mymr", "၁,၂၃၄.၅၆", "၊"},
This construct gives the following compiler warning:

tsdcfmsy.cpp:259:40: warning: conversion from string literal to 'char *' is
deprecated [-Wc++11-compat-deprecated-writable-strings]

Suggestions on how to write this in a better way?

[markus.icu, 2017-07-27 20:52:17]

API LGTM

Didn't review impl or test.

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
File icu4c/source/test/intltest/tsdcfmsy.cpp (right):

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
icu4c/source/test/intltest/tsdcfmsy.cpp:262: {"my", "mymr", "၁,၂၃၄.၅၆", "၊"},
On 2017/07/27 06:15:59, sffc wrote:
Make it
    static const char *cases[8...

----
FYI

We don't have a strict rule for
    type* name
vs.
    type *name
but ICU mostly uses the latter because it's how the type system actually works.

[sffc, 2017-07-27 21:14:45]

Prelimiary feedback from Markus

[sffc, 2017-07-27 21:15:21]

The API proposal has been sent.

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
File icu4c/source/test/intltest/tsdcfmsy.cpp (right):

https://codereview.appspot.com/323340043/diff/1/icu4c/source/test/intltest/ts...
icu4c/source/test/intltest/tsdcfmsy.cpp:262: {"my", "mymr", "၁,၂၃၄.၅၆", "၊"},
On 2017/07/27 20:52:17, markus.icu wrote:
> On 2017/07/27 06:15:59, sffc wrote:
> Make it
>     static const char *cases[8...
> 
> ----
> FYI
> 
> We don't have a strict rule for
>     type* name
> vs.
>     type *name
> but ICU mostly uses the latter because it's how the type system actually
works.

Done.

[markus.icu, 2017-08-08 23:44:38]

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
File icu4c/source/i18n/dcfmtsym.cpp (right):

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
icu4c/source/i18n/dcfmtsym.cpp:117: : UObject(),
funky indentation

Not quite Google style, but I think the most readable is:

DecimalFormatSymbols::DecimalFormatSymbols(const Locale& loc, const
NumberingSystem& ns,
                                           UErrorCode& status) :
        UObject(), locale(loc) {
    initialize(...);
}

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
icu4c/source/i18n/dcfmtsym.cpp:367: if (ns == NULL) {
Please use nullptr in new code.

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/unicode...
File icu4c/source/i18n/unicode/dcfmtsym.h (right):

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/unicode...
icu4c/source/i18n/unicode/dcfmtsym.h:193: * the numbering system by name.
Yoshito asked in the proposal doc what happens when two different numbering
systems are specified -- via locale ID as well as "ns". I assume "ns" wins.
Please reply to his comment in the doc, and document here (and in Java).

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/test/intltes...
File icu4c/source/test/intltest/tsdcfmsy.cpp (right):

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/tsdcfmsy.cpp:254: static const char *cases[8][4] = {
Please add a test case with conflicting numbering systems.

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/tsdcfmsy.cpp:266: Locale loc =
Locale::createFromName(cases[i][0]);
Locale loc(cases[i][0]);

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/test/intltes...
icu4c/source/test/intltest/tsdcfmsy.cpp:271: UnicodeString
expected1(cases[i][2], "utf-8");
Please change the test cases to a struct, use const char16_t * for the third and
fourth fields, and use the u prefix on the Unicode string literals. Then you can
construct the UnicodeString objects directly from there without conversion.

Optional: Rename expected1 and expected2 to something slightly more meaningful.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java
(right):

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:139: *
the numbering system by name.
See C++ comment about conflicting inputs.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:144: *
@draft ICU 60
Add the @provisional line, copy from another draft method somewhere (e.g., class
Edits).

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:155: *
the numbering system by name.
ditto conflict

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:160: *
@draft ICU 60
ditto @provisional

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
File
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java
(right):

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:315:
Object[][] cases = {
Please add a test case for conflicting numbering systems.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:316:
{new ULocale("en"), NumberingSystem.getInstanceByName("latn"), "1,234.56", ';'},
More readable would be to just store strings here and construct the ULocale and
NumberingSystem in the loop.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:329:
String expected1 = (String) cas[2];
Optional: Rename expected1 and expected2 to something slightly more meaningful.

[sffc, 2017-08-17 02:47:02]

Second round of feedback

[sffc, 2017-08-17 02:47:53]

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
File icu4c/source/i18n/dcfmtsym.cpp (right):

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
icu4c/source/i18n/dcfmtsym.cpp:117: : UObject(),
On 2017/08/08 23:44:37, markus.icu wrote:
> funky indentation
> 
> Not quite Google style, but I think the most readable is:
> 
> DecimalFormatSymbols::DecimalFormatSymbols(const Locale& loc, const
> NumberingSystem& ns,
>                                            UErrorCode& status) :
>         UObject(), locale(loc) {
>     initialize(...);
> }

I think my original indentation was consistent with the other constructors.  I
changed all four to something closer to what you suggested.

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/dcfmtsy...
icu4c/source/i18n/dcfmtsym.cpp:367: if (ns == NULL) {
On 2017/08/08 23:44:38, markus.icu wrote:
> Please use nullptr in new code.

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/unicode...
File icu4c/source/i18n/unicode/dcfmtsym.h (right):

https://codereview.appspot.com/323340043/diff/20001/icu4c/source/i18n/unicode...
icu4c/source/i18n/unicode/dcfmtsym.h:193: * the numbering system by name.
On 2017/08/08 23:44:38, markus.icu wrote:
> Yoshito asked in the proposal doc what happens when two different numbering
> systems are specified -- via locale ID as well as "ns". I assume "ns" wins.
> Please reply to his comment in the doc, and document here (and in Java).

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
File icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java
(right):

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:139: *
the numbering system by name.
On 2017/08/08 23:44:38, markus.icu wrote:
> See C++ comment about conflicting inputs.

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:144: *
@draft ICU 60
On 2017/08/08 23:44:38, markus.icu wrote:
> Add the @provisional line, copy from another draft method somewhere (e.g.,
class
> Edits).

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:155: *
the numbering system by name.
On 2017/08/08 23:44:38, markus.icu wrote:
> ditto conflict

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/classes/core/s...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormatSymbols.java:160: *
@draft ICU 60
On 2017/08/08 23:44:38, markus.icu wrote:
> ditto @provisional

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
File
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java
(right):

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:315:
Object[][] cases = {
On 2017/08/08 23:44:38, markus.icu wrote:
> Please add a test case for conflicting numbering systems.

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:316:
{new ULocale("en"), NumberingSystem.getInstanceByName("latn"), "1,234.56", ';'},
On 2017/08/08 23:44:38, markus.icu wrote:
> More readable would be to just store strings here and construct the ULocale
and
> NumberingSystem in the loop.

Done.

https://codereview.appspot.com/323340043/diff/20001/icu4j/main/tests/core/src...
icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/IntlTestDecimalFormatSymbols.java:329:
String expected1 = (String) cas[2];
On 2017/08/08 23:44:38, markus.icu wrote:
> Optional: Rename expected1 and expected2 to something slightly more
meaningful.

Done.

[markus.icu, 2017-08-17 21:54:00]

LGTM

[sffc, 2017-08-17 23:49:17]

On 2017/08/17 21:54:00, markus.icu wrote:
> LGTM

Committed revision 40345.