ticket:11018: Support CLDR time separator (ICU4C)

ticket:11018: Support CLDR time separator (ICU4C)

This change will treat the ':' character in patterns as a symbol
(instead of as a literal) and if the Locale defines timeSeparator then
use that character instead when formatting and accept both when parsing.

It will also define the '.' as an alternate time separator, that will be
accepted when parsing in lenient mode.

This is one character more strict than before, when in lenient mode also
the '-' was accepted as time separator instead of ':'.

R=rocketman@google.com

Committed: http://bugs.icu-project.org/trac/changeset/36897

[tkeep, 2014-12-06 00:11:41]

Ok, I've started reviewing, but got to go because of holiday party.

On 5 December 2014 at 15:23, <roubert@google.com> wrote:

> Reviewers: tkeep,
>
> Description:
> ticket:11018: Support CLDR time separator (ICU4C)
>
> This change will treat the ':' character in patterns as a symbol
> (instead of as a literal) and if the Locale defines timeSeparator then
> use that character instead when formatting and accept both when parsing.
>
> R=rocketman@google.com
>
> Please review this at https://codereview.appspot.com/180610043/
>
> Affected files (+239, -28 lines):
>   M source/i18n/calendar.cpp
>   M source/i18n/dtfmtsym.cpp
>   M source/i18n/gregocal.cpp
>   M source/i18n/smpdtfmt.cpp
>   M source/i18n/unicode/dtfmtsym.h
>   M source/i18n/unicode/ucal.h
>   M source/i18n/unicode/udat.h
>   M source/test/cintltst/cdattst.c
>   M source/test/intltest/dtfmttst.h
>   M source/test/intltest/dtfmttst.cpp
>   M source/tools/toolutil/udbgutil.cpp
>
>
>


-- 
100 / 9900 = .01010101010101010101...

100 / 9801 = .01020304050607080910...

100 / 9899 = .01010203050813213455... (Fibonacci)

[tkeep, 2014-12-06 00:12:46]

More to come.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp
File source/i18n/dtfmtsym.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:770: static const UnicodeString
defaultTimeSeparator(':');
Not sure this will work on all the architectures we support. This essentially
creates a UnicodeString as a global variable but limits the scope to just this
function. While having int, char *, and other primitive types as globals is
allowed, global objects generally are not allowed in ICU code because there are
no standards on when or how C++ initializes these. While this may work on your
machine, there is no guarantee it will work everywhere.

Instead create a global UChar array. You can make a UnicodeString wrap a global
UChar array. Generally in ICU, the caller passes a reference to a UnicodeString
which you mutate and your function returns a non-const reference to that mutated
UnicodeString.

[tkeep, 2014-12-08 17:46:54]

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp
File source/i18n/dtfmtsym.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1627: if (U_SUCCESS(tempStatus) && ns.isValid()) {
It should be enough to just check U_SUCCESS(tempStatus). If unumsys_open returns
NULL without setting an error, its a bug and you should file a ticket.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1629: const char *nsName =
unumsys_getName(ns.getAlias());
This is a cpp file, so you should be able to use the NumberingSystem C++ API
directly. The C API is used by Apple for their objective C and others who have
no choice but to use plain C.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1647: if (fTimeSeparator == NULL) {
You could just do if (fTimeSeparator == "")

if fTimeSeparator were a UnicodeString.

https://codereview.appspot.com/180610043/diff/1/source/i18n/smpdtfmt.cpp
File source/i18n/smpdtfmt.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/smpdtfmt.cpp#newc...
source/i18n/smpdtfmt.cpp:1967: || (ch == 0x3A))) { // [A-Za-z:]
Optional: This is different than the JAVA code which does just a simple range
check. We like to keep logic consistent between C++ and JAVA because we have to
maintain both in parallel. 

I prefer this implementation over the JAVA one because it is more tightly
written.

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym.h
File source/i18n/unicode/dtfmtsym.h (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym....
source/i18n/unicode/dtfmtsym.h:739: UnicodeString*  fTimeSeparator;
Can this just be UnicodeString fTimeSeparator?

If there are situations where fTimeSeparator will not be set, maybe just use
empty string?

We like to avoid heap allocations when possible because they make the code more
complicated, they can fail with out of memory, and there is some overhead.

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym....
source/i18n/unicode/dtfmtsym.h:884: 
If you use UnicodeString fTimeSeparator, you won't need this function.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1321: const LocalPointer<const TimeZone>
tz(TimeZone::createTimeZone("CET"));
Documentation states that TimeZone::createTimeZone is guaranteed to return a
non-NULL value. On failure it returns a pointer to a mutable clone of
getUnknown(). You can check for this by comparing with getUnknown(), but likely
your test will fail with the unknown timezone anyway.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1343: "\\u0669\\u060C\\u0665\\u0662 \\u0645",
-1, US_INV).unescape();
Optional: Since you initialize expectedArab for the first time, the copy
constructor may give slightly better performance.

UnicodeString expectedArab(
    UnicodeString(....).unescape());

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1356: errln((UnicodeString)"FAIL: "
Optional: You can use assertEquals("", expectedLatn, actualLatn);

Or assertEquals("some assert description", expectedLatn, actualLatn);

Our test assert function documentation is somewhat hidden. You just have to
learn about these as you look at ICU tests.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1362: }
As with the JAVA, I don't how the feature you added, the (:) as a pattern
character, is being tested here.

Did these tests fail before you added your change? Consider explaining in
documentation how this is testing the (:) character.

Beware that CLDR data changes all the time. Just because choosing the "ar"
locale today exercises your new code, doesn't mean it will exercise your code
after the next CLDR release.

[roubert (google), 2014-12-10 22:54:21]

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp
File source/i18n/dtfmtsym.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:770: static const UnicodeString
defaultTimeSeparator(':');
On 2014/12/06 00:12:46, tkeep wrote:
> Not sure this will work on all the architectures we support. This essentially
> creates a UnicodeString as a global variable but limits the scope to just this
> function. While having int, char *, and other primitive types as globals is
> allowed, global objects generally are not allowed in ICU code because there
are
> no standards on when or how C++ initializes these. While this may work on your
> machine, there is no guarantee it will work everywhere.
> 
> Instead create a global UChar array. You can make a UnicodeString wrap a
global
> UChar array. Generally in ICU, the caller passes a reference to a
UnicodeString
> which you mutate and your function returns a non-const reference to that
mutated
> UnicodeString.
> 

Done.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1627: if (U_SUCCESS(tempStatus) && ns.isValid()) {
On 2014/12/08 17:46:53, tkeep wrote:
> It should be enough to just check U_SUCCESS(tempStatus). If unumsys_open
returns
> NULL without setting an error, its a bug and you should file a ticket.

Done.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1629: const char *nsName =
unumsys_getName(ns.getAlias());
On 2014/12/08 17:46:53, tkeep wrote:
> This is a cpp file, so you should be able to use the NumberingSystem C++ API
> directly. The C API is used by Apple for their objective C and others who have
> no choice but to use plain C.

Done.

https://codereview.appspot.com/180610043/diff/1/source/i18n/dtfmtsym.cpp#newc...
source/i18n/dtfmtsym.cpp:1647: if (fTimeSeparator == NULL) {
On 2014/12/08 17:46:53, tkeep wrote:
> You could just do if (fTimeSeparator == "")
> 
> if fTimeSeparator were a UnicodeString.

Acknowledged.

https://codereview.appspot.com/180610043/diff/1/source/i18n/smpdtfmt.cpp
File source/i18n/smpdtfmt.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/smpdtfmt.cpp#newc...
source/i18n/smpdtfmt.cpp:1967: || (ch == 0x3A))) { // [A-Za-z:]
On 2014/12/08 17:46:53, tkeep wrote:
> I prefer this implementation over the JAVA one because it is more tightly
> written.

OK, I've updated the Java code now.

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym.h
File source/i18n/unicode/dtfmtsym.h (right):

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym....
source/i18n/unicode/dtfmtsym.h:739: UnicodeString*  fTimeSeparator;
On 2014/12/08 17:46:53, tkeep wrote:
> Can this just be UnicodeString fTimeSeparator?
> 
> If there are situations where fTimeSeparator will not be set, maybe just use
> empty string?
> 
> We like to avoid heap allocations when possible because they make the code
more
> complicated, they can fail with out of memory, and there is some overhead.

Done.

https://codereview.appspot.com/180610043/diff/1/source/i18n/unicode/dtfmtsym....
source/i18n/unicode/dtfmtsym.h:884: 
On 2014/12/08 17:46:53, tkeep wrote:
> If you use UnicodeString fTimeSeparator, you won't need this function.

Done.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
File source/test/intltest/dtfmttst.cpp (right):

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1321: const LocalPointer<const TimeZone>
tz(TimeZone::createTimeZone("CET"));
On 2014/12/08 17:46:54, tkeep wrote:
> Documentation states that TimeZone::createTimeZone is guaranteed to return a
> non-NULL value. On failure it returns a pointer to a mutable clone of
> getUnknown(). You can check for this by comparing with getUnknown(), but
likely
> your test will fail with the unknown timezone anyway.

Done.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1343: "\\u0669\\u060C\\u0665\\u0662 \\u0645",
-1, US_INV).unescape();
On 2014/12/08 17:46:54, tkeep wrote:
> Optional: Since you initialize expectedArab for the first time, the copy
> constructor may give slightly better performance.
> 
> UnicodeString expectedArab(
>     UnicodeString(....).unescape());

In this case, when initializing an object with another object of the same type,
both expressions, A aa(a) and A aa = a, mean exactly the same thing. They both
invoke the copy-constructor.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1356: errln((UnicodeString)"FAIL: "
On 2014/12/08 17:46:54, tkeep wrote:
> Optional: You can use assertEquals("", expectedLatn, actualLatn);
> 
> Or assertEquals("some assert description", expectedLatn, actualLatn);
> 
> Our test assert function documentation is somewhat hidden. You just have to
> learn about these as you look at ICU tests.

Done.

https://codereview.appspot.com/180610043/diff/1/source/test/intltest/dtfmttst...
source/test/intltest/dtfmttst.cpp:1362: }
On 2014/12/08 17:46:54, tkeep wrote:
> As with the JAVA, I don't how the feature you added, the (:) as a pattern
> character, is being tested here.

The answer to that will be exactly the same as for the Java test:

On 2014/12/01 18:36:18, tkeep wrote:

> How does this test the functionality that you added?

By creating two locales that only differ in the value of the numbers keyword,
where one is set to a numbering system other than the default, it then can
verify that date formatters using these two different locales use different time
separators.

> Even if 'ar' currently uses ':' in its pattern, CLDR data may change so
> that later 'ar' no longer explicitly uses ':'. Then your changes won't
> be tested anymore.

It is true that any change to the time separator in CLDR for 'ar' would break
this test, but such a change would cause a visible test failure requiring the
strings expectedArab and expectedLatn to be updated, so at least there is no
risk of the test silently becoming a no-op.

Do you know of any simple way to supply a test like this with custom CLDR data
for testing?

[roubert (google), 2014-12-12 17:02:59]

I've now updated this change with new handling of the time separator in lenient
parsing, accepting also an alternate separator. Please take another look.

[roubert (google), 2014-12-15 22:51:02]

Do you have any further comments about this change, or is it good to submit now?

[tkeep, 2014-12-16 15:15:49]

I only wish I understood how your unit tests test the functionality you
added. I don't see in your added unit test where you actually use a ':' so
that it is part of the pattern string.

On 15 December 2014 at 14:51, <roubert@google.com> wrote:
>
> Do you have any further comments about this change, or is it good to
> submit now?
>
> https://codereview.appspot.com/180610043/
>


-- 
100 / 9900 = .01010101010101010101...

100 / 9801 = .01020304050607080910...

100 / 9899 = .01010203050813213455... (Fibonacci)

[roubert (google), 2014-12-16 15:47:11]

On 2014/12/16 15:15:49, tkeep wrote:
> I only wish I understood how your unit tests test the functionality you
> added. I don't see in your added unit test where you actually use a ':' so
> that it is part of the pattern string.

The pattern string comes from CLDR data (for I haven't been able to figure out a
way to test this with data provided by the test itself; would that be possible
somehow?) so you'll have to look in CLDR if you want to see the definitions of
the time separators and the pattern string.

But you don't have to, for at the heart of the test case you can find this:

    const UnicodeString expectedArab = UnicodeString(
            "\\u0669\\u060C\\u0665\\u0662 \\u0645", -1, US_INV).unescape();

    const UnicodeString expectedLatn = UnicodeString(
            "9:52 \\u0645", -1, US_INV).unescape();

Notice how the same pattern string (the one from CLDR, whatever it is), is
expected to result in different date separator symbols in the output, U+060C (،)
and U+003A (:) respectively, depending on what number system is used. That could
not happen unless the formatting logic replaces the time separator used in the
pattern with the one appropriate for the number system used.

Do you follow me?

[tkeep, 2014-12-16 17:52:44]

Sound good. At least put a TODO / comment stating that your functionality
is tested implicitly by the locale. DecimalFormat does offer setPattern()
that lets you explicitly set a pattern, but you can worry about this some
other time. Just add the TODO and I will be fine with your changes.


On 16 December 2014 at 07:47, <roubert@google.com> wrote:
>
> On 2014/12/16 15:15:49, tkeep wrote:
>
>> I only wish I understood how your unit tests test the functionality
>>
> you
>
>> added. I don't see in your added unit test where you actually use a
>>
> ':' so
>
>> that it is part of the pattern string.
>>
>
> The pattern string comes from CLDR data (for I haven't been able to
> figure out a way to test this with data provided by the test itself;
> would that be possible somehow?) so you'll have to look in CLDR if you
> want to see the definitions of the time separators and the pattern
> string.
>
> But you don't have to, for at the heart of the test case you can find
> this:
>
>     const UnicodeString expectedArab = UnicodeString(
>             "\\u0669\\u060C\\u0665\\u0662 \\u0645", -1,
> US_INV).unescape();
>
>     const UnicodeString expectedLatn = UnicodeString(
>             "9:52 \\u0645", -1, US_INV).unescape();
>
> Notice how the same pattern string (the one from CLDR, whatever it is),
> is expected to result in different date separator symbols in the output,
> U+060C (،) and U+003A (:) respectively, depending on what number system
> is used. That could not happen unless the formatting logic replaces the
> time separator used in the pattern with the one appropriate for the
> number system used.
>
> Do you follow me?
>
> https://codereview.appspot.com/180610043/
>


-- 
100 / 9900 = .01010101010101010101...

100 / 9801 = .01020304050607080910...

100 / 9899 = .01010203050813213455... (Fibonacci)

[roubert (google), 2014-12-16 19:26:16]

On 2014/12/16 17:52:44, tkeep wrote:
> At least put a TODO / comment stating that your functionality
> is tested implicitly by the locale.

But that's nothing particular to TestFormattingLocaleTimeSeparator(), almost all
tests in this file depend on CLDR data. Look at TestLocaleDateFormat() just
above, for example, it's exactly the same test logic: create two different
Locale objects, use them for formatting, verify the resulting strings, the
resulting strings being entirely dependent on CLDR data.

For a TODO comment to be useful, I think there needs to be a reason to believe
that whatever it is that at the time prevents from writing something better is
going to change in the future, preferrably with some way of tracking or
determining when that has been done. Otherwise such comments are just noise.

I know that you on the icu-core mailing list earlier this year proposed adding
functionality to use custom CLDR data from inside of tests:

https://sourceforge.net/p/icu/mailman/message/32448556/

Did that lead to anything? Are you going to work on that? Is there a ticket
tracking such work?

> DecimalFormat does offer setPattern() that lets you explicitly set a pattern,

But as long as it isn't possible to also build also the rest of the Locale from
inside the test it'll still be dependent on CLDR data, so setting the pattern
explicitly doesn't alone bring very much.

[tkeep, 2014-12-18 21:55:30]

LGTM

On 16 December 2014 at 11:26, <roubert@google.com> wrote:
>
> On 2014/12/16 17:52:44, tkeep wrote:
>
>> At least put a TODO / comment stating that your functionality
>> is tested implicitly by the locale.
>>
>
> But that's nothing particular to TestFormattingLocaleTimeSeparator(),
> almost all tests in this file depend on CLDR data. Look at
> TestLocaleDateFormat() just above, for example, it's exactly the same
> test logic: create two different Locale objects, use them for
> formatting, verify the resulting strings, the resulting strings being
> entirely dependent on CLDR data.
>
> For a TODO comment to be useful, I think there needs to be a reason to
> believe that whatever it is that at the time prevents from writing
> something better is going to change in the future, preferrably with some
> way of tracking or determining when that has been done. Otherwise such
> comments are just noise.
>
> I know that you on the icu-core mailing list earlier this year proposed
> adding functionality to use custom CLDR data from inside of tests:
>
> https://sourceforge.net/p/icu/mailman/message/32448556/
>
> Did that lead to anything? Are you going to work on that? Is there a
> ticket tracking such work?
>
>  DecimalFormat does offer setPattern() that lets you explicitly set a
>>
> pattern,
>
> But as long as it isn't possible to also build also the rest of the
> Locale from inside the test it'll still be dependent on CLDR data, so
> setting the pattern explicitly doesn't alone bring very much.
>
> https://codereview.appspot.com/180610043/
>


-- 
100 / 9900 = .01010101010101010101...

100 / 9801 = .01020304050607080910...

100 / 9899 = .01010203050813213455... (Fibonacci)

[roubert (google), 2014-12-19 07:19:50]

Committed patchset #4 (id:60001) manually as 36897 (presubmit successful).